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

Convert Lombok @Value classes to Java records #319

Conversation

kevin0x90
Copy link
Contributor

@kevin0x90 kevin0x90 commented Oct 14, 2023

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

  • I've added unit tests to cover both positive and negative cases
  • I've added the license header to any new files through ./gradlew licenseFormat
  • I've used the IntelliJ IDEA auto-formatter on affected files

@kevin0x90 kevin0x90 changed the title #141 add draft of lombok value to java record conversion #141 Lombok value to java record conversion Oct 14, 2023
@timtebeek
Copy link
Contributor

This is looking very promising already! Thanks for getting this started. Could you run ./gradlew lF to add the license headers? That would make the tests run here.

@kevin0x90
Copy link
Contributor Author

This is looking very promising already! Thanks for getting this started. Could you run ./gradlew lF to add the license headers? That would make the tests run here.

Hi,

Sorry for the late reply and thank you for adding the copyright information.

I added more changes to approach the defined acceptability.
During the replacement of the getter methods I ran into some type problems. Maybe you have an idea how to prevent this?

@kevin0x90 kevin0x90 marked this pull request as ready for review October 16, 2023 20:08
Copy link
Contributor

@timtebeek timtebeek left a 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.

@timtebeek timtebeek added the recipe Recipe requested label Oct 18, 2023
@timtebeek timtebeek linked an issue Oct 19, 2023 that may be closed by this pull request
@timtebeek timtebeek changed the title #141 Lombok value to java record conversion Convert Lombok @Value classes to Java records Oct 19, 2023
@kevin0x90
Copy link
Contributor Author

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.

I did not yet try out the Moderne CLI but will give it a try for my sample project.

@timtebeek
Copy link
Contributor

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. :)

@kevin0x90
Copy link
Contributor Author

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.

Copy link
Contributor

@timtebeek timtebeek left a 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 ?

@knutwannheden
Copy link
Contributor

I can take a look.

Copy link
Contributor

@knutwannheden knutwannheden left a 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!

@timtebeek timtebeek merged commit ffe1b69 into openrewrite:main Oct 27, 2023
1 check passed
@timtebeek
Copy link
Contributor

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.

@timtebeek
Copy link
Contributor

Ran it on a first few projects; think we might need some minor tweaks and polish. Example diff below
image

Two quick findings:

  1. the fields converted to constructor args lack the corresponding indentation
  2. we remove the @Value annotation anywhere in the compilation unit, not just on the record

@timtebeek
Copy link
Contributor

Follow up now in:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
recipe Recipe requested
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Migrate from @lombok.Value to Java records
3 participants