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

Upgrade Error Prone 2.17.0 -> 2.18.0 #455

Merged
merged 1 commit into from
Jan 11, 2023

Conversation

Stephan202
Copy link
Member

@Stephan202 Stephan202 commented Jan 9, 2023

Suggested commit message:

Upgrade Error Prone 2.17.0 -> 2.18.0 (#455)

See:
- https://github.com/google/error-prone/releases/tag/v2.18.0
- https://github.com/google/error-prone/compare/v2.17.0...v2.18.0
- https://github.com/PicnicSupermarket/error-prone/compare/v2.17.0-picnic-1...v2.18.0-picnic-1

@Stephan202 Stephan202 added this to the 0.8.0 milestone Jan 9, 2023
@Stephan202 Stephan202 requested a review from rickie January 9, 2023 20:53
@github-actions
Copy link

github-actions bot commented Jan 9, 2023

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@Picnic-Bot
Copy link
Contributor

Suggested commit message:

Upgrade Error Prone 2.17.0 -> 2.18.0

Comment on lines +1590 to +1592
<!-- Yoda conditions are not always more
readable than the alternative. -->
-Xep:YodaCondition:OFF
Copy link
Member Author

Choose a reason for hiding this comment

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

To be sure: we can discuss enabling this, of course. Maybe I should just get used to it. Even so: the resultant changes also require careful null analysis.

Copy link
Member

Choose a reason for hiding this comment

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

I'm on the fence about this. Enabling this would result in better/easier readable code IMO. What concerns do you have with enabling this 👀? (Other than getting used to it 😉)

In any case, it would make sense to enable this in a separate PR (especially if we want to make the next internal PSM release).

Copy link
Member Author

Choose a reason for hiding this comment

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

What concerns do you have with enabling this 👀 (Other than getting used to it 😉 )

Well the question is whether it's just "getting used to", or that in some cases readability actually suffers. Separately from that it might force us to introduce additional explicit null checks, which would be plain awkward.

Copy link
Member

Choose a reason for hiding this comment

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

We could open a PR where we enable the check and go over the results and make a decision for EPS there? It is hard to reason about it now if we don't know how "bad" it'll be.

Copy link
Member Author

Choose a reason for hiding this comment

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

Draft PRs are free ;)

That said, I'd also like to see the arguments brought up during the internal discussion on this topic. 🧑‍🎓

@rickie rickie force-pushed the renovate/version.error-prone-orig branch from 773c038 to 39bddbe Compare January 11, 2023 10:17
@github-actions
Copy link

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@rickie rickie force-pushed the renovate/version.error-prone-orig branch from 39bddbe to ba1b1b1 Compare January 11, 2023 17:50
@github-actions
Copy link

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@rickie rickie merged commit 79356ac into master Jan 11, 2023
@rickie rickie deleted the renovate/version.error-prone-orig branch January 11, 2023 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants