-
Notifications
You must be signed in to change notification settings - Fork 775
[rewrite] Add Java8toJava11
#5328
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
base: master
Are you sure you want to change the base?
Conversation
| } | ||
|
|
||
| private static @Nullable Tree suppressibleNode(TreePath path, VisitorState state) { | ||
| return StreamSupport.stream(path.spliterator(), false) |
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.
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.
If there is interest in using (a subset of) our Refaster rules, IMO it'd make more sense to directly import them from Error Prone Support, instead of going via OpenRewrite.
CC: @Stephan202
| </dependency> | ||
| <dependency> | ||
| <groupId>jakarta.inject</groupId> | ||
| <artifactId>jakarta.inject-api</artifactId> |
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.
java migration
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'm not sure what this means, why was this change made?
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.
yes this is kind of new to everyone but seems best practise:
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.
its needed as part of upcomming javax to jakarta bump, so its kind of required:
PRO tip: here its not included due to maven ignorance but this is kind of bug it system design.
cushon
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.
Were all of the changes in this PR generated by openrewrite using the rewrite.yml config you added?
Some of them look unrelated, like the formatting changes to the pom.xml files and some of the Java files.
|
yes sorry its all related to some individual recipe. Im going to supplement and extract to make it transparent and mergeable. Thank you. |
rickie
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.
Some thoughts :)
rewrite.yml
Outdated
| displayName: Apply Java & Maven best practices | ||
| description: Comprehensive code quality recipe combining modernization, security, and best practices. | ||
| recipeList: | ||
| - org.openrewrite.java.RemoveUnusedImports |
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.
There already exists an Error Prone check for this.
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.
yes, also all tools have bugs and or different perspectives, never say never.
rewrite.yml
Outdated
| recipeList: | ||
| - org.openrewrite.java.RemoveUnusedImports | ||
| - org.openrewrite.java.format.NormalizeLineBreaks | ||
| - org.openrewrite.java.format.RemoveTrailingWhitespace |
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.
Would say this cannot occur given the use of Google Java Format.
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.
yes, also all tools have bugs and or different perspectives, never say never.
spot uses google or palant which is google. (zero trust principle)
d69eb5f to
8749941
Compare
Java8toJava11Java8toJava11
8749941 to
86da496
Compare
always helpful and welcome. Let me know if something to adjust, kind regards. |
86da496 to
bc4bed3
Compare
|
thanks for the fixup. rebase: |
Signed-off-by: Vincent Potucek <[email protected]>
bc4bed3 to
6896381
Compare
related to:
UpgradeToJava17diffplug/spotless#2636Rewrite:UpgradeToJava17checkstyle/checkstyle#17730rewritesupport forerrorprone.refasterrules#5195