Skip to content

Conversation

@dellis1972
Copy link
Contributor

If a user provides lint checks which are not supported by
the current lint version it will error.

Invalid id or ategory "StatckFieldLeak"

This is normally ok. But in our build system we sometimes end up
on a bot which has an older version of lint. And in this case
the test fails, which in reality it should ignore the id we
are trying to use.

So this commit addds some validation to the Disabled and Enabled
checks. If its not supported we will remove the check and issue
a warning.

@dellis1972
Copy link
Contributor Author

build


foreach (var issue in DisabledIssuesByVersion) {
if (lintToolVersion < issue.Value) {
Regex issueReplaceRegex = new Regex ($"{issue.Key}(,)?");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of (,)?, I think this should use $@"\b{issue.Key}\b" (word boundaries), so that e.g. UnusedResourcesAreFun isn't matched against UnusedResources. \b would also "match" ,.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need the (,)? because we want to remove a ending comma with the string replace.

Foo,Foo1,Foo2

if we don't include the comma we end up with

Foo,,Foo2

or

Foo,Foo1,

both of which lint will complain about. That said adding the word boundaries should work too.

foreach (var issue in DisabledIssuesByVersion) {
if (lintToolVersion < issue.Value) {
Regex issueReplaceRegex = new Regex ($"{issue.Key}(,)?");
if (!string.IsNullOrEmpty (DisabledIssues) && DisabledIssues.Contains (issue.Key)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block and the following block are largely identical in structure, they just deal with different variables. This should be factored out into a method.


string CleanIssues (string issueToRemove, Version lintToolVersion, string issues, string issuePropertyName)
{
Regex issueReplaceRegex = new Regex ($"\b{issueToRemove}\b(,)?");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need the ,. The \b word boundary will still be triggered at a comma.

var r = new Regex(@"\bFoo\b");
r.Match ("Foo").Success;     // true
r.Match ("Foo,").Success;    // true
r.Match ("Foo,Bar").Success; // true
r.Match ("FooBar").Success;  // false

If a user provides lint checks which are not supported by
the current lint version it will error.

	Invalid id or ategory "StatckFieldLeak"

This is normally ok. But in our build system we sometimes end up
on a bot which has an older version of lint. And in this case
the test fails, which in reality it should ignore the id we
are trying to use.

So this commit addds some validation to the Disabled and Enabled
checks. If its not supported we will remove the check and issue
a warning.
@jonpryor jonpryor merged commit 6bfa199 into dotnet:master Nov 7, 2017
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request May 5, 2022
Changes: dotnet/java-interop@2a882d2...4787e01

  * dotnet/java-interop@4787e017: [Java.Base-Tests] Test Java-to-Managed invocations for Java.Base (dotnet#975)
  * dotnet/java-interop@59716252: [build] `main` *conceptually* targets .NET 7 (dotnet#978)
  * dotnet/java-interop@61cdb40d: [generator] Fix reserved keywords binary search (dotnet#977)
jonpryor added a commit that referenced this pull request May 6, 2022
Changes: dotnet/java-interop@2a882d2...843f3c7

  * dotnet/java-interop@843f3c78: [Java.Base-Tests] Use $(UtilityOutputFullPath)/jcw-gen.dll (#979)
  * dotnet/java-interop@4787e017: [Java.Base-Tests] Test Java-to-Managed invocations for Java.Base (#975)
  * dotnet/java-interop@59716252: [build] `main` *conceptually* targets .NET 7 (#978)
  * dotnet/java-interop@61cdb40d: [generator] Fix reserved keywords binary search (#977)
jonpryor added a commit that referenced this pull request May 6, 2022
Changes: dotnet/java-interop@2a882d2...843f3c7

  * dotnet/java-interop@843f3c78: [Java.Base-Tests] Use $(UtilityOutputFullPath)/jcw-gen.dll (#979)
  * dotnet/java-interop@4787e017: [Java.Base-Tests] Test Java-to-Managed invocations for Java.Base (#975)
  * dotnet/java-interop@59716252: [build] `main` *conceptually* targets .NET 7 (#978)
  * dotnet/java-interop@61cdb40d: [generator] Fix reserved keywords binary search (#977)
@github-actions github-actions bot locked and limited conversation to collaborators Feb 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants