Skip to content
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

LUCENE-9497: add google error prone checks #1828

Closed
wants to merge 10 commits into from

Conversation

dweiss
Copy link
Contributor

@dweiss dweiss commented Sep 4, 2020

No description provided.

…all the warnings, otherwise it stops on the first one.
…ison. error_prone uses built-in SuppressWarnings (which is kind of neat).
}

tasks.withType(JavaCompile) { task ->
options.errorprone.errorproneArgs = [
Copy link
Contributor

Choose a reason for hiding this comment

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

I would also like to see options.errorprone.disableWarningsInGeneratedCode = true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suggested the same to Varun.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add this ? I probably missed it in my PR

tasks.withType(JavaCompile) { task ->
options.errorprone.errorproneArgs = [
// test
'-Xep:ExtendingJUnitAssert:OFF',
Copy link
Contributor

Choose a reason for hiding this comment

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

Personal style, but I think options.errorprone { disable 'ExtendingJUnitAssert' }
is more clear than using -Xep?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am okay with either of the two styles. Ideally we'd want this list to get much shorter soon :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Varun - feel free to take this branch (or patch) and roll it out on yours. I didn't intend it to be committed, I just wanted to show what's needed for it to compile and work.

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good! I'll take the current branch and add two things and then commit the code

  1. options.errorprone.disableWarningsInGeneratedCode = true
  2. CHANGES entry

@dweiss dweiss requested a review from vthacker September 4, 2020 15:27
@@ -523,6 +523,7 @@ public void clear() {
* @throws NullPointerException
* if the given map is <code>null</code>.
*/
@SuppressWarnings("ReferenceEquality")
Copy link
Contributor

Choose a reason for hiding this comment

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

This was the example you wanted to try out on how to suppress legitimate warnings of ReferenceEquality ( or any other warnings ) when we start enabling the checks ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly. I wanted the pr to include an example of how this can be done.

Copy link
Contributor

@vthacker vthacker left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants