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

Fix #457: Refactor javax.annotation to ChangePackage #525

Merged
merged 1 commit into from
Aug 7, 2024

Conversation

melloware
Copy link
Contributor

@melloware melloware commented Aug 7, 2024

What's changed?

Fix #457: Refactor javax.annotation to ChangePackage

Change from renaming individual Classes to ChangePackage. Not sure why it was going to all this trouble for individual names when one ChangePackage will do.

What's your motivation?

Checklist

@timtebeek timtebeek self-requested a review August 7, 2024 12:36
@timtebeek timtebeek added the enhancement New feature or request label Aug 7, 2024
@timtebeek
Copy link
Contributor

Thanks for the simplification! I do notice these hints in the git history:

But perhaps we can undo that unintended change with a simple addition of

  - org.openrewrite.java.ChangePackage:
      oldPackageName: jakarta.annotation.processing
      newPackageName: javax.annotation.processing

That would keep this straightforward while still resolving #457

@melloware
Copy link
Contributor Author

YUCK....let me research.

@melloware
Copy link
Contributor Author

@timtebeek i made the change but i think maybe the README.MD needs to be update with build instructions? gradlew build or gradlew install i can't figure out how to build the snapshot in my local /m2 repo?

@timtebeek
Copy link
Contributor

@timtebeek i made the change but i think maybe the README.MD needs to be update with build instructions? gradlew build or gradlew install i can't figure out how to build the snapshot in my local /m2 repo?

Ah yes I can see it might take some getting used to with Gradle; the instructions are documented here:

TLDR: ./gradlew publishToMavenLocal does what mvn install would otherwise do.

Thanks again for providing the fix here! Great to have that troublesome behavior of ChangeType-in-bulk-on-larger-projects resolved.

@timtebeek timtebeek merged commit d57a809 into openrewrite:main Aug 7, 2024
2 checks passed
@melloware melloware deleted the R457 branch August 7, 2024 16:51
@mr1chter
Copy link

mr1chter commented Aug 26, 2024

Hi! not sure if I'm right here or if I should open a proper bug report. This PR kinda broke my jakarta ee 10 migration, because we use a few spotbugs annotations that also live under javax.annotations (which are part of JSR305 (mostly frozen?), we get them via com.google.code.findbugs:annotations, which gets pulled in by spotbugs-annotations).

I think in our case those annotations could just as well get migrated to the associated spotbugs annotations (edu.umd.cs.findbugs.annotations) but for now they are there and while migrating them to their jakarta counterparts does not really hurt in the most part (spotbugs supports it), there are a few that are not part of the jakarta namespace (because they were also not part of java ee 8), like javax.annotation.Nonnegative. When I ran the migration previously, those were left alone, but now they also get rewritten to jakarta.annotation.Nonnegative which does not compile.

I'm not sure if this use case is important enough to warrant a bug report though, since someone else exporting classes in the javax.annotation namespace is weird enough.

@melloware
Copy link
Contributor Author

yeah that is weird they should not be doing that :) I would report a bug to SpotBugs/FindBugs to have them fix it.

@timtebeek
Copy link
Contributor

Thanks for the heads up @mr1chter ; Indeed we would not have expected spotbugs to add recipes to the javax.annotation package. I'd report it there first.

For your particular case you can write your own recipe that undoes the changes made here with the addition of this step

  - org.openrewrite.java.ChangeType:
      oldFullyQualifiedTypeName: jakarta.annotation.Nonnegative
      newFullyQualifiedTypeName: javax.annotation.Nonnegative

I hope that helps!

@iparadiso
Copy link

iparadiso commented Oct 4, 2024

Hi @timtebeek. I'm encountering this problem. There are a bunch of annotations in com.google.code.findbugs:jsr305 that are transformed. I tried your proposed workaround in a follow-on recipe, but I'm having problems with it working.

Does org.openrewrite.java.ChangeType and org.openrewrite.java.ChangePackage require type resolution in order to work? If so, then the following might not work since jakarta.annotation.Nonnegative type cannot be resolved as it doesn't exist on the classpath.

 - org.openrewrite.java.ChangeType:
      oldFullyQualifiedTypeName: jakarta.annotation.Nonnegative
      newFullyQualifiedTypeName: javax.annotation.Nonnegative

@knutwannheden
Copy link
Contributor

Maybe we should add an exceptions option to ChangePackage.

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

Successfully merging this pull request may close these issues.

@PostConstruct import being removed from beans
5 participants