-
Notifications
You must be signed in to change notification settings - Fork 80
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
Convert Lombok @Value
classes to Java records
#319
Convert Lombok @Value
classes to Java records
#319
Conversation
This is looking very promising already! Thanks for getting this started. Could you run |
Hi, Sorry for the late reply and thank you for adding the copyright information. I added more changes to approach the defined acceptability. |
…ion of the lombok toString method
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.
Quick first round of feedback; really like that you took this on! Hope to get this closer to a merge soon, and start trying it out on projects.
Did you ever look towards using the Moderne CLI? That might allow you to run it locally across projects quickly, after a first round of building serialized LSTs. Not a requirement, but could give you feedback on how this works on your own projects without the repeated wait for the lossless semantic tree models to be built.
src/main/java/org/openrewrite/java/migrate/lombok/LombokValueToRecord.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/migrate/lombok/LombokValueToRecord.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/migrate/lombok/LombokValueToRecord.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/migrate/lombok/LombokValueToRecord.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/migrate/lombok/LombokValueToRecord.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/migrate/lombok/LombokValueToRecord.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/migrate/lombok/LombokValueToRecordTest.java
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/migrate/lombok/LombokValueToRecordTest.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/migrate/lombok/LombokValueToRecord.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/migrate/lombok/LombokValueToRecord.java
Outdated
Show resolved
Hide resolved
@Value
classes to Java records
I did not yet try out the Moderne CLI but will give it a try for my sample project. |
src/main/java/org/openrewrite/java/migrate/lombok/LombokValueToRecord.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/migrate/lombok/LombokValueToRecord.java
Outdated
Show resolved
Hide resolved
…ady existing code
…vor of MaybeUsesImport
Finally found some more time to review; haven't yet gotten to the missing type information, but did find some simplifications and a case not yet covered around static fields. Not quite sure what records allow there, but might be safest to just not convert those cases. I've also grouped the tests into a nested class, such that it's easier to spot what should and should not be supported. Let me know if you'd like to fix that failing test yourself, or if you'd at any point want to hand this over. :) |
Hi, I will fix the failing test and keep on working on this branch whenever I find the time to do so. |
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.
Looks good to me! Thanks again.
Any last minute feedback @knutwannheden or @joanvr ?
I can take a look. |
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 just added a few small remarks. Apart from that I think this looks very good!
src/main/java/org/openrewrite/java/migrate/lombok/LombokValueToRecord.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/migrate/lombok/LombokValueToRecord.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/migrate/lombok/LombokValueToRecord.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/migrate/lombok/LombokValueToRecord.java
Outdated
Show resolved
Hide resolved
Thanks a lot @kevin0x90 ! You can try out this change through our snapshot versions, pending the next release. I'll deploy it to https://app.moderne.io as well, and see how it performs against our own code base as well. |
Follow up now in: |
What's changed?
Adds a recipe for converting lombok value annotated classes to java records.
What's your motivation?
I am working on a case study to evaluate the effort and benefit of automated refactorings.
Anyone you would like to review specifically?
@timtebeek
Checklist
./gradlew licenseFormat