-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Enable ErrorProne checks and fix resulting findings #4744
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
Conversation
* Classes with equals calling `getClass()` may potentially problematic. I made them `final` where they were "effectively" `final` by only having private or package-private constructors. * I switched from `LinkedList` to `ArrayList` or `ArrayDeque` based on ErrorProne's recommendations.
| "BadImport", | ||
| "UnnecessaryLambda", | ||
| "AnnotateFormatMethod", | ||
| "StringSplitter", | ||
| "DoNotCallSuggester", | ||
| "InlineMeSuggester", | ||
| "ImmutableEnumChecker", | ||
| "MissingSummary" |
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.
Some of those checks do sound interesting, though, especially UnnecessaryLambda.
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 might be worth adding a TODO comment for such "nice to fix" so somebody could pick them later.
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.
I have added comments explaining my decision to disable these checks in 73e3dad.
sormuras
left a comment
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.
All textual updates, including in API documentation and suppressions, look reasonable.
Code changes seem to be covered by existing tests.
|
There's https://errorprone.info/bugpattern/PackageLocation which has "suggestion" level by default, and it is probably worth having as |
Thanks for the suggestion! Added in b8357c3 |
| var value = random.nextLong(); | ||
| if (value == Long.MIN_VALUE) { | ||
| // ensure Math.abs returns positive value | ||
| value++; | ||
| } | ||
| return Math.abs(value); |
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.
By the way, Java 17 has random.nextLong(Long.MAX_VALUE). Have you considered it instead?
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.
Took me a while (too long) to find it: https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html#nextLong(long)
Classes with equals calling
getClass()may potentially problematic.I made them
finalwhere they were "effectively"finalby onlyhaving private or package-private constructors.
I switched from
LinkedListtoArrayListorArrayDequebased onErrorProne's recommendations.
I hereby agree to the terms of the JUnit Contributor License Agreement.
Definition of Done
@APIannotations