-
Notifications
You must be signed in to change notification settings - Fork 75
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
Remove Jakarta annotation dependency when moving to Jakarta packages #487
Remove Jakarta annotation dependency when moving to Jakarta packages #487
Conversation
src/test/java/org/openrewrite/java/migrate/jakarta/JavaxToJakartaTest.java
Show resolved
Hide resolved
recipeList: | ||
- org.openrewrite.java.dependencies.DependencyInsight: | ||
groupIdPattern: jakarta.annotation | ||
artifactIdPattern: jakarta.annotation-api |
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.
Preconditions are the typical way to limit what files recipes operate on. That would look something like this
recipeList: | |
- org.openrewrite.java.dependencies.DependencyInsight: | |
groupIdPattern: jakarta.annotation | |
artifactIdPattern: jakarta.annotation-api | |
preconditions: | |
- org.openrewrite.java.dependencies.DependencyInsight: | |
groupIdPattern: jakarta.annotation | |
artifactIdPattern: jakarta.annotation-api | |
recipeList: |
I'm not sure about the use of DependencyInsight here though, as we likely only want to remove the dependency if it's already available transitively. That might need some custom logic. 🤔
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, not just found.
We need find in transitively "only", is this possible with DependencyInsight?
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.
Not right now; we do have an onlyDirect
option on that recipe, but perhaps we'd need to add an onlyTransitive
:
https://docs.openrewrite.org/recipes/maven/search/dependencyinsight#dependencies-in-use
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 see, guess we can only postpone this PR until DependencyInsight support onlyTransitive
? 😢
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.
Possibly yes. Although that might not be too difficult as we already know the depth there, since we add that to the data table we produce.
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.
How?
Just like my comment 1. said, it's gone.
I can submit PR with the failed test if you want.
This is a workaround, reason:openrewrite#481 (comment)
I check if contain spring boot starter instead, because 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.
Thanks a lot for restoring this functionality. Should be good to see the jakarta annotation removed in the common case of using a spring-boot-starter.
@timtebeek |
Hi! It's not yet I'm the latest release, but should be in the latest snapshot version. We release every two weeks. Hope that helps! |
What's changed?
Add a recipe to revert the jakarta.annotation-api added by org.openrewrite.java.migrate.javax.AddCommonAnnotationsDependencies
What's your motivation?
Fix #481
jakarta.annotation-api
dependency added in Java 11 migration should be removed when migrating to Jakarta #481Anyone you would like to review specifically?
@timtebeek
Checklist