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.

Sign up for GitHub

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

Jump to bottom

Use Tree.randomId() to construct LST elements, not UUID.randomUUID() #3784

Merged
merged 14 commits into from
Feb 4, 2024
Merged

Use Tree.randomId() to construct LST elements, not UUID.randomUUID() #3784

merged 14 commits into from
Feb 4, 2024

Conversation

TheMarvelFan
Copy link
Contributor

@TheMarvelFan TheMarvelFan commented Dec 7, 2023 •
edited by timtebeek
Loading

Referencing Issue #3757

What's changed?

A recipe has been added to modify all references to UUID.randomUUID() to Tree.randomId(). This recipe touches only these references., and does not modify the other classes.

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've read and applied the recipe conventions and best practices
  • I've used the IntelliJ IDEA auto-formatter on affected files

@TheMarvelFan TheMarvelFan marked this pull request as draft December 7, 2023 10:01
@timtebeek timtebeek self-requested a review December 7, 2023 10:14
@timtebeek
Copy link
Contributor

Hi @TheMarvelFan , thanks for getting this started! Your current implementation is a bit too broad. We'd only want to replace these method calls when they are an argument to an LST constructor call.

@TheMarvelFan
Changed the recipe so that it only changes UUID.randomID() to Tree.ra…
20e0944 
…ndomId() only when it is used as a parameter in an LST constructor.
@TheMarvelFan
Copy link
Contributor Author

@timtebeek
Please check it now and suggest changes if required.

timtebeek
timtebeek reviewed Dec 7, 2023
View reviewed changes
rewrite-java/src/main/java/org/openrewrite/java/ReplaceUUIDRandomId.java Outdated
Comment on lines 71 to 72
public J.Literal visitLiteral(J.Literal literal, ExecutionContext ctx) {
J.Literal l = super.visitLiteral(literal, ctx);
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of visitLiteral you'll want to override visitMethodInvocation, then use a MethodMatcher to check that it's a LST element constructor call, and if is then inspect the first argument, and if that is a call the UUID.randomUUID, then replace that first argument using ListUtils.mapFirst. Does that help?

@TheMarvelFan
Copy link
Contributor Author

Hi @timtebeek,
I changed the recipe methods as requested. Please review the files and suggest more changes if required.

@timtebeek timtebeek mentioned this pull request Dec 18, 2023
Merged
3 tasks
@TheMarvelFan
Copy link
Contributor Author

Hi @timtebeek,
What's the status of this PR?

@timtebeek
Copy link
Contributor

Hi @timtebeek, What's the status of this PR?

Hi @TheMarvelFan ! I've not had time to dive in fully here; I've applied a quick few suggestions to hopefully get the tests going, but don't expect to find time until next week to look at this again. Thanks for getting this started! Hope we can improve this and make it part of the checks that we run on every pull request.

@timtebeek timtebeek added the recipe Requested Recipe label Dec 28, 2023
traceyyoshima
traceyyoshima reviewed Jan 1, 2024
View reviewed changes
Copy link
Contributor

@traceyyoshima traceyyoshima left a comment

Choose a reason for hiding this comment

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

Hi @TheMarvelFan , thank you for opening a PR! I've left some comments for consideration.

rewrite-java/src/main/java/org/openrewrite/java/ReplaceUUIDRandomId.java Outdated Show resolved Hide resolved
rewrite-java/src/main/java/org/openrewrite/java/ReplaceUUIDRandomId.java Outdated Show resolved Hide resolved
rewrite-java/src/main/java/org/openrewrite/java/ReplaceUUIDRandomId.java Outdated Show resolved Hide resolved
rewrite-java/src/main/java/org/openrewrite/java/ReplaceUUIDRandomId.java Outdated Show resolved Hide resolved
@timtebeek timtebeek marked this pull request as ready for review February 4, 2024 20:21
@timtebeek timtebeek linked an issue Feb 4, 2024 that may be closed by this pull request
Replace UUID.randomUUID() with Tree.randomId() as part of recipe best practices #3757
Closed
@timtebeek timtebeek changed the title Replace UUID.randomUUID() with Tree.randomId() Use Tree.randomId() to construct LST elements, not UUID.randomUUID() Feb 4, 2024
timtebeek
timtebeek approved these changes Feb 4, 2024
View reviewed changes
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.

Thanks for the initial help here @TheMarvelFan and thanks for the detailed review comments @traceyyoshima ; I've gone ahead and applied the suggestions, such that we can start to use this recipe in our automated code reviews. 🎉

@timtebeek timtebeek merged commit f3d4cf0 into openrewrite:main Feb 4, 2024
1 check passed
@timtebeek
Copy link
Contributor

Also applied it to the OpenRewrite projects just now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@traceyyoshima traceyyoshima traceyyoshima left review comments

@knutwannheden knutwannheden knutwannheden left review comments

@timtebeek timtebeek timtebeek approved these changes

Assignees

@TheMarvelFan TheMarvelFan

Labels
recipe Requested Recipe
Projects
OpenRewrite
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace UUID.randomUUID() with Tree.randomId() as part of recipe best practices
4 participants
@TheMarvelFan @timtebeek @knutwannheden @traceyyoshima