-
Notifications
You must be signed in to change notification settings - Fork 90
feat: normalize effective getter methods #631
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
src/test/java/org/openrewrite/java/migrate/lombok/NormalizeGetterTest.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/migrate/lombok/LombokUtils.java
Outdated
Show resolved
Hide resolved
a7f556d
to
bf8d66e
Compare
…terTest.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
65097c4
to
9442836
Compare
9442836
to
5605d68
Compare
Hi @timtebeek, could you please take another look at this? I've applied all changes I've seen you do on the other PRs. |
Thanks for making those adjustments! That's indeed been holding back a review because of the time investment needed to adjust the PRs. We'll have another look! |
src/main/java/org/openrewrite/java/migrate/lombok/NormalizeGetter.java
Outdated
Show resolved
Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
src/main/java/org/openrewrite/java/migrate/lombok/NormalizeGetter.java
Outdated
Show resolved
Hide resolved
…ter.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.
This one is looking good @timo-a , thanks a lot for your patience throughout. The only thing I'm wondering is whether NormalizeGetter
is descriptive enough for folks, or if we should perhaps use something like AdoptLombokGetterMethodNames
, as could be clearer in the intent.
Yeah, I guess normalize is a bit vague. I've implemented your suggestion. |
Thanks a lot! We'll merge as soon as the tests pass, and then this should go out with the release ~Wednesday |
What's changed?
Adds recipe that renames "effective getter methods" to the way lombok would name them.
What's your motivation?
When a team starts without lombok, their getter and setter methods will over time stop adhering to the way lombok names them. Maybe the team never followed the lombok naming, maybe fields got renamed but not their getter methods. Maybe previously more complex methods morph into just being just getters but keep their old name...
This recipe will normalize those methods names, so the converter can find them.
This is a separate recipe on purpose. Reasons:
Anything in particular you'd like reviewers to focus on?
Please evaluate the edge cases. I've estimated that the effort needed to cover them is not worth it, but maybe you can see a way.
Also, I overloaded a method in
LombokUtils
with the original version. Maybe the methods can be merged?Anyone you would like to review specifically?
Have you considered any alternatives or workarounds?
Any additional context
Checklist