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

Recipes to migrate from Javax, Jakarta and JetBrains nullability annotations to JSpecify #541

Merged
merged 18 commits into from
Aug 30, 2024

Conversation

sullis
Copy link
Contributor

@sullis sullis commented Aug 26, 2024

What's changed?

add recipe for jspecify annotations

What's your motivation?

jspecify 1.0.0 was released in July 2024

https://jspecify.dev/blog/release-1.0.0/

https://www.infoq.com/news/2024/08/jspecify-java-nullability/

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

n/a

Anyone you would like to review specifically?

n/a

Have you considered any alternatives or workarounds?

n/a

Any additional context

jspecify mailing list
https://groups.google.com/g/jspecify-discuss/c/h73MkdhAPew

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

sullis and others added 7 commits August 26, 2024 15:26
…specifyTest.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…specifyTest.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…specifyTest.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…specifyTest.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
- org.openrewrite.java.dependencies.AddDependency:
groupId: org.jspecify
artifactId: jspecify
version: latest.release
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be best to add an onlyIfUsing expression here, such that we only add the dependency where needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just added onlyIfUsing but it doesn't seem to be working.

I tried this:

 onlyIfUsing: org.jspecify.annotations..*

I also tried this:

 onlyIfUsing: org.jspecify.annotations.*

Copy link
Contributor

Choose a reason for hiding this comment

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

There's a bit of an implementation detail here with how that onlyIfUsing is implemented, and how scanning recipes work. Scanning recipes run a first separate scan across all source files, before making informed changes downstream. We only do a single scan across all files, as otherwise we could potentially have to do a lot of passes through very large projects, slowing things down considerably. The AddDependency recipe is a scanning recipe, that evaluates the onlyIfUsing in that initial pass through, seeing the files as they are before the ChangeType recipes make their changes. That means that if we evaluate org.jspecify.annotations.*, then it's not (yet) seeing those annotations being used, so it's not adding a dependency, even if effectively in the end we do end up using those annotations. The trick then is to look at the "before" situation instead, looking for each of the old annotations, and adding the JSpecify dependency if found, as we expect those annotations to be converted in subsequent steps. Hope that helps explain the issue!

@sullis sullis marked this pull request as ready for review August 27, 2024 18:00
@wakingrufus
Copy link

Nice! Any thoughts on including org.jetbrains.annotations.*?

@sullis
Copy link
Contributor Author

sullis commented Aug 28, 2024

Nice! Any thoughts on including org.jetbrains.annotations.*?

Great idea. I am adding MigrateFromJetbrainsAnnotations

@sullis
Copy link
Contributor Author

sullis commented Aug 28, 2024

@timtebeek can you look at the CI build failure?

MigrateToJspecifyTest > migrateFromJavaxAnnotationApiToJspecify() FAILED
    org.opentest4j.AssertionFailedError: [The recipe should have made the following change to "pom.xml"] 

@sullis
Copy link
Contributor Author

sullis commented Aug 30, 2024

@timtebeek can you look at the CI build failure?

MigrateToJspecifyTest > migrateFromJavaxAnnotationApiToJspecify() FAILED
    org.opentest4j.AssertionFailedError: [The recipe should have made the following change to "pom.xml"] 

Perhaps I am using 'onlyIfUsing' incorrectly. I'm not 100% sure if my implementation is correct.

@timtebeek timtebeek changed the title add jspecify recipe Recipes to migrate from Javax, Jakarta and JetBrains nullability annotations to JSpecify Aug 30, 2024
@timtebeek
Copy link
Contributor

@timtebeek can you look at the CI build failure?

MigrateToJspecifyTest > migrateFromJavaxAnnotationApiToJspecify() FAILED
    org.opentest4j.AssertionFailedError: [The recipe should have made the following change to "pom.xml"] 

Perhaps I am using 'onlyIfUsing' incorrectly. I'm not 100% sure if my implementation is correct.

All good; took me a little while longer to get to this than I had hoped, but I've fixed the issue just now with a write up in #541 (comment)

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 a lot for adding these recipes @sullis ! Should make it a lot easier for folks to make the switch as well. :)

@timtebeek timtebeek merged commit 57a7c3d into openrewrite:main Aug 30, 2024
2 checks passed
@sullis sullis deleted the sean/jspecify branch August 30, 2024 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
recipe Recipe requested
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants