-
Notifications
You must be signed in to change notification settings - Fork 39
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
Upgrade Error Prone 2.18.0 -> 2.19.1 #621
Conversation
Suggested commit message:
|
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'll have a look at updating the fork. Locally I fixed the compilation issues, though there are some test failures to look into. Lastly, we should also drop StringCaseLocaleUsage
.
I'll create a PR for that :). |
Doesn't it belong in this PR? 🤔 |
Lol, for some reason I thought I clicked the PSM PR, but yes we should haha. |
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.
Added one more commit. I created a new release of the fork, and the build now passes with -DskipTests
on JDK 17. There are still test issues, which I'll look into next.
// XXX: Also flag `String::toLowerCase` and `String::toUpperCase` method references. For these cases | ||
// the suggested fix should introduce a lambda expression with a parameter of which the name does | ||
// not coincide with the name of an existing variable name. Such functionality should likely be | ||
// introduced in a utility class. |
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 added an entry to my local list; ideally we do still pick this up.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
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.
Added a commit. The build and tests now pass on JDK 17 and 20, but the JDK 11 is broken due to google/error-prone#3895.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
1 similar comment
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
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.
Added a commit to fix the JDK 11 build. But given that these changes will break Refaster template compatibility with Error Prone 2.18.0 (due to serialization format changes), while users may not upgrade to Error Prone 2.19.0 due to google/error-prone#3895, I'm somewhat torn about the speed with which we should merge this PR 🤔
@AfterTemplate | ||
@CanIgnoreReturnValue | ||
BugCheckerRefactoringTestHelper after(BugCheckerRefactoringTestHelper helper) { | ||
return helper; | ||
} |
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 nice to open a PR upstream to skip suggesting @CanIgnoreReturnValue
on @AfterTemplate
. After all, such methods are not really meant to be invoked.
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'm totally up for that, will create it tomorrow morning 🦺.
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.
Consider first checking the internal upgrade PR; as I mention there, there are a bunch of other (non-Refaster) cases in which adding @CanIgnoreReturnValue
also doesn't really make sense.
getDescription(delegate)) | ||
.overrideSeverity(overrideSeverity(getSeverity(delegate), context)) |
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.
PItest warns about this call not being covered, but that's because the tests are in another module. If I drop this line then RefasterTest
does fail.
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.
Added a commit to upgrade to Error Prone 2.19.1, which comes with a fix for google/error-prone#3895.
One thing we should perhaps do (in a separate PR) is document that Error Prone Support's Refaster rules are only compatible with the version of Error Prone against which they're compiled. Ideally we document that version on the website. 🤔
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Edited/Blocked NotificationRenovate will not automatically rebase this PR, because it does not recognize the last commit author and assumes somebody else may have edited the PR. You can manually request rebase by checking the rebase/retry box above. ⚠ Warning: custom changes will be lost. |
3d79983
to
0c7dbca
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
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.
Nice changes! Besides the introduced CanIgnoreReturnValue
it LGTM. Have code working for excluding Refaster rules but will definitely first check the internal PR.
Should we add one of the keywords like |
Filed: PicnicSupermarket/error-prone#60. Do we add XXX references when I open the PR upstream and merge then? WDYT? |
0c7dbca
to
8e97904
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
8e97904
to
78264c3
Compare
Rebased. Will merge once built. Decided to leave the |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Kudos, SonarCloud Quality Gate passed! |
This PR contains the following updates:
2.18.0
->2.19.0
2.18.0
->2.19.0
2.18.0
->2.19.0
2.18.0
->2.19.0
2.18.0
->2.19.0
2.18.0
->2.19.0
Release Notes
google/error-prone
v2.19.0
: Error Prone 2.19.0Compare Source
New Checkers:
NotJavadoc
StringCaseLocaleUsage
UnnecessaryTestMethodPrefix
Fixes issues: #956, #3504, #3654, #3703, #3731, #3737, #3760, #3779, #3796, #3809, #3813
Full Changelog: google/error-prone@v2.18.0...v2.19.0