Issue #14137: Disable Error Prone Support checks #14192
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -237,6 +237,20 @@ | |||||||
| <error-prone.version>2.24.0</error-prone.version> | ||||||||
| <error-prone-support.version>0.14.0</error-prone-support.version> | ||||||||
| <doxia.version>1.12.0</doxia.version> | ||||||||
| <error-prone.configuration-args> | ||||||||
| <!-- Reason at https://github.com/checkstyle/checkstyle/issues/8252. --> | ||||||||
| -Xep:JUnitClassModifiers:OFF | ||||||||
| <!-- Reason at https://github.com/checkstyle/checkstyle/issues/8252. --> | ||||||||
| -Xep:JUnitMethodDeclaration:OFF | ||||||||
|
Contributor
There was a problem hiding this comment.
Suggested change
|
||||||||
| <!-- We prefer different style of writing tests. --> | ||||||||
| -Xep:JUnitValueSource:OFF | ||||||||
|
Contributor
There was a problem hiding this comment. Why not activate this pattern?
Member
There was a problem hiding this comment. We do not use this pattern of test design, we eliminating all tests that are unit tests, we do all tests by full checkstyle execution.
Member
Contributor
Author
There was a problem hiding this comment. For now I added: Let me know if you prefer something else. A quick search showed me that in the project there are only 2 parameterized tests, both use |
||||||||
| <!-- Until https://github.com/checkstyle/checkstyle/issues/14194. --> | ||||||||
| -Xep:LexicographicalAnnotationListing:OFF | ||||||||
|
Member
There was a problem hiding this comment.
Contributor
Author
There was a problem hiding this comment. You are correct. Note the slight difference, we want to enable LexicographicalAnnotationAttributeListing, but here I disabled the LexicographicalAnnotationListing. That check simply sorts annotations lexicographically, which is not that desirable in most cases.
Member
There was a problem hiding this comment. @rnveach , @nrmancuso , do we want to try sorting of annotations?
Member
There was a problem hiding this comment. We can disable it for now, and enable it in separate PR.
Member
There was a problem hiding this comment. When we introduce new tools, we usually suppress all one by one , explicitly and in next PRs, enable one by one.
Member
There was a problem hiding this comment. In next PRs we can reuse https://github.com/checkstyle/checkstyle/tree/master/config/error-prone-suppressions to activate rule and simply put all/controversial violations to suppression and deal with them separately. At least nobody will merge a code to introduce more violations, there are a lot concurrent PRs now. I just try to share a way to move quick and not be blocked by complicated cases.
Contributor
There was a problem hiding this comment. As for annotation sorting, I prefer to have simple
Member
There was a problem hiding this comment. Ordering usually becomes matter, if there are numerous annotations. @nrmancuso , do you mean you like piramid style of annotations? The most parametrized the bottom
Contributor
There was a problem hiding this comment.
Yes! This is a good way to describe it. I feel like the basic
Member
There was a problem hiding this comment. Ok, good to know, but this discussion should not block this PR, please review PR to let us unleash disablement of some rules. We can reconsider disablement in separate PR. Piramid annotation check sound like good idea to discuss in separate issue.
Contributor
There was a problem hiding this comment.
Suggested change
|
||||||||
| <!-- This check is not finalized. --> | ||||||||
| -Xep:MethodReferenceUsage:OFF | ||||||||
|
Contributor
There was a problem hiding this comment. We shouldn't have many (any?) violations on this one, since we have an IDEA inspection for this, can we activate this pattern?
Member
There was a problem hiding this comment. If no violations present now, let's activate. I highly recommend to merge basic suppression sooner and deal with all activations in separate PRs.
Contributor
Author
There was a problem hiding this comment. The only problem is that this is the only rule in Error Prone Support that is not ready for use yet. For historical reasons this check is already "available" like this, but it's better for now to disable it. We also disabled it internally.
Contributor
Author
There was a problem hiding this comment. For now I added this: |
||||||||
| <!-- We prefer to qualify all static method calls with class name. --> | ||||||||
| -Xep:StaticImport:OFF | ||||||||
|
Contributor
There was a problem hiding this comment.
Suggested change
Member
There was a problem hiding this comment. Yes, We do most of code review in phone browsers, no ide, so code must be explicit. |
||||||||
| </error-prone.configuration-args> | ||||||||
| </properties> | ||||||||
|
|
||||||||
| <dependencies> | ||||||||
|
|
@@ -2380,7 +2394,7 @@ | |||||||
| <arg>-Xpkginfo:always</arg> | ||||||||
| <arg>-XDcompilePolicy=simple</arg> | ||||||||
| <arg> | ||||||||
| -Xplugin:ErrorProne ${error-prone.configuration-args} | ||||||||
| </arg> | ||||||||
| </compilerArgs> | ||||||||
| <annotationProcessorPaths> | ||||||||
|
|
@@ -2438,7 +2452,8 @@ | |||||||
| <arg>-XDcompilePolicy=simple</arg> | ||||||||
| <arg> | ||||||||
| -Xplugin:ErrorProne \ | ||||||||
| -XepExcludedPaths:.*[\\/]resources[\\/].* \ | ||||||||
| ${error-prone.configuration-args} | ||||||||
| </arg> | ||||||||
| </compilerArgs> | ||||||||
| <annotationProcessorPaths> | ||||||||
|
|
@@ -2451,7 +2466,7 @@ | |||||||
| <groupId>tech.picnic.error-prone-support</groupId> | ||||||||
| <artifactId>error-prone-contrib</artifactId> | ||||||||
| <version>${error-prone-support.version}</version> | ||||||||
| </path> | ||||||||
romani marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
| </annotationProcessorPaths> | ||||||||
| </configuration> | ||||||||
| </execution> | ||||||||
|
|
||||||||
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 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.
We need to close this issue, we are not going to follow this style for now, may be in future we will, but I don't see much reason.
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.
Issue is closed.
Please make comment
<!-- reason at https://github.com/checkstyle/checkstyle/issues/8252 -->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.
Reasoning is sound: we do not want to allow/encourage sharing of test methods across test classes.
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.
Changing visibility does not protect again reuse, we are open for any reuse of public method, usually it is nonsense and not possible to have any reason.