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

Recipe AddSerialAnnotationToserialVersionUID #247

Merged
merged 10 commits into from
Jul 13, 2024
Merged

Conversation

jbessels
Copy link
Contributor

@jbessels jbessels commented Jan 21, 2024

What's changed?

Added recipe AddSerialAnnotationToserialVersionUID. Ran recipe AddSerialVersionUidToSerializable on the firm repo's and quite a few files were changed. Since Java 14 the @serial annotation exists. The current recipe does not add that annotation.

What's your motivation?

This is my first recipe ever and a learning experience to get the ball rolling. That is why I created a new recipe instead of creating a feature request ticket that asks to add that @serial annotation in the AddSerialAnnotationToserialVersionUID recipe. My recipe will only add a @serial but will not change/fix the existing code. The correct format is
private static final long serialVersionUID = 1;
Sometimes private and/or static is missing, it works but not according to the standard. Such cases will not be corrected by my recipe. For that, there is recipe AddSerialVersionUidToSerializable if a user chooses to use that.

Anything in particular you'd like reviewers to focus on?

As its my first recipe and the Moderne API is new to me I will make mistakes. No need to be gentle, just tell me what I'm doing wrong and/or what I'm doing correct (if so). I need the recipe to be elegant, fast and applying the best practices of Moderne.

Anyone you would like to review specifically?

@timtebeek offered to contribute/help me with this recipe.

Have you considered any alternatives or workarounds?

My first approach was based upon AddSerialAnnotationToserialVersionUID and the main logic was in the visitClassDeclaration() method. Turned out that using that approach its not possible or not easy/elegant to put a @serial above a serialVersionUID line. It was always put above the class. @timtebeek suggested to use visitVariableDeclarations() instead as it would it easier and being able to insert that @serial above the mentioned line.

Any additional context

None

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

@timtebeek timtebeek marked this pull request as draft January 21, 2024 15:46
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.

Couple first quick suggestion to help readability and prevent conflicts when we work on the same files.

Comment on lines 104 to 105
// Yes I know deprecated: varDecls.getAllAnnotations()
List<J.Annotation> allAnnotations = varDecls.getAllAnnotations();
Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed elsewhere we've already adopted the annotation service; you'll want to look into that before we merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will be corrected if I still need it with the new approach visitVariableDeclarations() approach.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • src/main/java/org/openrewrite/staticanalysis/ReplaceDuplicateStringLiterals.java
    • lines 250-249

timtebeek and others added 3 commits July 4, 2024 11:11
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@timtebeek timtebeek marked this pull request as ready for review July 13, 2024 00:49
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.

Hi @jbessels ; Thanks for getting this started! Ended up applying the necessary changes to complete this one such that we can close out this old PR. Feel free to look at the changes I've made since to see what you can still learn from seeing this implemented. In particular note how we're no longer overriding visitClassDeclaration for the bulk of the work, but instead have pushed that down to visitVariableDeclarations.

@timtebeek timtebeek merged commit 9dc027c into openrewrite:main Jul 13, 2024
2 checks passed
@timtebeek
Copy link
Contributor

@melloware
Copy link

Should this be part of the Java17 automatic migration? Or part of the 14 migration that happens as part of the 17 migration Recipe?

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

Successfully merging this pull request may close these issues.

3 participants