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

Adding all org.glassfish.main migrations to jakarta #517

Merged
merged 4 commits into from
Jul 29, 2024

Conversation

sgartner03
Copy link
Contributor

What's changed?

Added migrations for all Java EE APIs under the organization org.glassfish.main. Added a migration for java.ejb:javax.ejb to jakarta.ejb. These changes are implemented in the org.openrewrite.java.migrate.jakarta.JavaxMigrationToJakarta Recipe.

What's your motivation?

https://rewriteoss.slack.com/archives/C01A843MWG5/p1721228381110469

Any additional context

Should I implement tests for each case? As far as I see the other migrations of this recipe don't have tests and I'm not sure what for I would be testing other than typos anyways.

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

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.

Great to see, thanks! Couple of quick questions as I went over the artifactIds; if you could answer those I think we can merge soon after.

For Yaml only recipes it's not always necessary to write a test; in some cases that might bring to light dependencies that can't be found, as might have been the case here. No need to add them now I'd say.

@timtebeek timtebeek added enhancement New feature or request recipe Recipe requested labels Jul 26, 2024
Co-authored-by: Tim te Beek <timtebeek@gmail.com>
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 for the work done here and educating me on the changes. :)

@timtebeek timtebeek merged commit 8376fac into openrewrite:main Jul 29, 2024
2 checks passed
@sgartner03
Copy link
Contributor Author

Thanks for your help! However, I did an oopsie by thinking your thumbs-up in the discussion for org.glassfish:jakarta.faces meant that I should re-add the migration with another target. I think we should revert commit 124cb01 to re-delete the jsf migration.

timtebeek added a commit that referenced this pull request Jul 29, 2024
@timtebeek
Copy link
Contributor

Thanks! Dropped now in 61ed4bc

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

Successfully merging this pull request may close these issues.

2 participants