-
Notifications
You must be signed in to change notification settings - Fork 290
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
Update to Error Prone 2.21.1 #797
Conversation
@@ -21,16 +21,16 @@ jobs: | |||
epVersion: 2.4.0 | |||
- os: macos-latest | |||
java: 11 | |||
epVersion: 2.20.0 | |||
epVersion: 2.21.0 |
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.
Seeing this change, I wonder if there is a way to define this version as a constant once in this yml and reference it in all these jobs and the check below, like we do inside gradle :)
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.
Unfortunately this is not possible. You can define variables in an env
context but you can't use them when specifying properties in the matrix
, see here.
* processed by {@link #onMatchTopLevelClass(NullAway, ClassTree, VisitorState, | ||
* Symbol.ClassSymbol)} | ||
*/ | ||
@Initializer |
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.
Just out of curiosity, was there any issue marking onMatchTopLevelClass(...)
itself as an initializer? Or is there a reason why that would be confusing? (I guess for all handlers, in general, we would expect that to be the first callback executed always?)
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.
You're right, just annotating onMatchTopLevelClass(...)
is better, and I switched to that. It's slightly strange since that method gets invoked multiple times (once for each class), but it does perform guaranteed initialization the first time it runs, so I think it's fine to annotate it as @Initializer
.
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.
LGTM. I assume it wasn't easy to get the EP version into a constant, then 😅
Mostly straightforward. Error Prone has a new NullableOptional check that warned on a couple instances in our code base. I changed these cases to use an
@Initializer
method which I think better captures what is going on. Will update the required CI checks before landing.