-
Notifications
You must be signed in to change notification settings - Fork 742
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add an option to exclude paths from being analyzed #599
Conversation
cc @msridhar |
This would also fix #511 |
@cushon ping on this |
Hello! We discussed this a bit: We believe that the combination of The flag allows error prone to skip generating warnings for code annotated with If you find that the generated code is consistently violating these error-level checks, we may want to consider reclassifying the checks, or fixing the code generator if the code it's generation contains the buggy code. |
Hey @nglorioso This is for more than just generated code. There are cases where we have vendor/third party code that needs to be part of the same sourceset. Also, not all code generators add the I think having an option like this would make it very flexible and let consumers suppress generated code issues etc. and also any other potential future use cases like during a migration when two modules are merged etc, to turn off EP on a particular path so it can be done incrementally etc. when dealing with large projects at scale |
Ping on this? |
Furthermore, this seems like something that would enable a gradual rollout of this tool on a legacy code that would otherwise generate hundreds of errors or warnings: |
*/ | ||
private boolean isExcludedPath(URI uri) { | ||
Pattern excludedPattern = errorProneOptions.getExcludedPattern(); | ||
return (excludedPattern != null) && excludedPattern.matcher(uri.toString()).matches(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use ASTHelpers.getFileNameFromUri
instead of URI.toString
? The former has special handling for jar entries, which I think it's possible to see here.
35c5b26
to
51c422b
Compare
@cushon updated as per your suggestion |
LGTM, but can you add test coverage? |
This was leftover code that shouldn't have made it here
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
I'm ok with my commits being included |
} else if (finishedCompilation(path.getCompilationUnit())) { | ||
// Otherwise this TaskEvent is for a ClassTree, and we can scan the whole | ||
// CompilationUnitTree once we've seen all the enclosed classes. | ||
transformer.get().apply(new TreePath(compilation), subContext, descriptionListener); | ||
if (!isExcludedPath(compilation.getSourceFile().toUri())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to de-duplicate this logic.
@@ -412,4 +435,12 @@ public static ErrorProneOptions processArgs(String[] args) { | |||
Preconditions.checkNotNull(args); | |||
return processArgs(Arrays.asList(args)); | |||
} | |||
|
|||
private static Pattern getExcludedPattern(Iterable<String> excludeSubStrings) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT about making the flag take a regex instead of a comma-separated list that gets processed into a regex? Making it an arbitrary regex avoids the need to define what the syntax is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense; will change
@@ -164,6 +166,20 @@ public void recognizesCompilingTestOnlyCode() { | |||
} | |||
|
|||
@Test | |||
public void recognizesExcludedPaths() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add an integration test that verifies the option is used to filter the compilation units that get analyzed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will look into it; is there an existing test whose structure I can copy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are test cases in ErrorProneJavaCompilerTest
that use flags to enable/disable checks and change severities. Maybe something like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks! I added a test case there.
Thanks! I'm merging this change internally and it will show up on github in the next day or so. Can you also create a PR to add documentation to http://errorprone.info/docs/flags? |
Awesome! Will create a documentation PR. |
Fixes #599 RELNOTES: Add an option to exclude paths from being analyzed ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=175049602
Fixes #599 RELNOTES: Add an option to exclude paths from being analyzed ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=175049602
Fixes #599 RELNOTES: Add an option to exclude paths from being analyzed ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=175049602
FTR: Not release as of right now, but #821 says about to... |
This adds an option to suppress some paths entirely from being analyzed by the error prone analyzer. This is particularly useful for generated code or third party code which consumers may not want to analyze.
Fixes #523
The input patterns are treated as regular expressions for path matching. For example:
-XepExcludedPaths:".*/build/generated/.*\.java"
will exclude all java files inside anybuild/generated/
directories