-
Notifications
You must be signed in to change notification settings - Fork 90
Reordering Annotations #544
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
a7188bd to
c25c65b
Compare
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
Efforts here can be continued as openrewrite/rewrite#5392 has been completed |
|
@Laurens-W no rush on this at all ofcourse, but it's odd to see that we introduce a newline for the simplest case, and there's weird indentation and shuffling of comments for the more complex case. The first I'd at least expect to be fixed; the second is optional. |
|
For the simple case I see this happen: The newly added recipe here calls autoformat on the method of which the NormalizeFormatVisitor does this upon visiting a method: Basically the |
|
Thanks for that context; in that case let's make it a little easier here and just keep whatever prefix was there before reshuffling. Would you agree? |
src/main/java/org/openrewrite/staticanalysis/ReorderAnnotations.java
Outdated
Show resolved
Hide resolved
timtebeek
left a comment
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.
Thanks all! I think this increment is ready to be merged, applied to all our projects, and enforced going forward. I'll wait for a second approval since I've made quite some changes still.
Laurens-W
left a comment
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.
LGTM
|
There's one change here that I'm not quite happy with, and was helpfully pointed out here: |

Reordering annotations (alphabetically only for now on method declarations alone).
Looking to define boundaries for what should be included in the changes for annotation reordering. As per the linked issue, the starting point would be a default alphabetical ordering of annotations (the ability to pass in what
Comparator<J.Annotation>to use will be upcoming). It's been suggested to start with just method declarations and move on from there after, but I'm open to thoughts and suggestions.What's changed?
ReorderAnnotationsrecipe for reordering annotations (default alphabetic) onJ.MethodDeclarationRelated issue with additional fixes that should be implemented first
Anyone you would like to review specifically?
@timtebeek
@greg-at-moderne
Checklist
Comparator<J.Annotation>you would like to use to sort the annotations on a given LST entry point