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

Joda-Time to Java time: Add support for Method Return Statement Migration #626

Merged
merged 4 commits into from
Dec 13, 2024

Conversation

amishra-u
Copy link
Contributor

What's changed?

  • Added support for safely migrating method return statements of the Joda type.
  • Migration Safety Criteria: A method return statement is considered safe to migrate if:
    • The return statement itself can be safely migrated.
    • All invocations of the method are safe to migrate.

Key Changes:

  1. Used a map of methodReferencedVars to track the referenced variables during assignment and in return expression. (A method invocation is unsafe for migration if any of referenced var is unsafe)
  2. Added a safeMethodMap to track the safe methods
  3. Added new test cases and updated existing ones to reflect the changes.

Not Implemented Yet:

  1. Migration of class variables.

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

@timtebeek timtebeek self-requested a review December 11, 2024 09:51
@timtebeek timtebeek added the enhancement New feature or request label Dec 13, 2024
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 @amishra-u ; thanks for the continued work here. I was slightly surprised to see methods migrated that have a public modifier and Joda-Time return type, but I suppose at some point we're going to have to make some changes if folks are to migrate at all. Let's hope folks understand that this is a one time migration that is hard to make guaranteed safe.

@timtebeek timtebeek merged commit 2c654e8 into openrewrite:main Dec 13, 2024
2 checks passed
@amishra-u
Copy link
Contributor Author

amishra-u commented Dec 13, 2024

Great to see @amishra-u ; thanks for the continued work here. I was slightly surprised to see methods migrated that have a public modifier and Joda-Time return type, but I suppose at some point we're going to have to make some changes if folks are to migrate at all. Let's hope folks understand that this is a one time migration that is hard to make guaranteed safe.

Yes, it needs to run on the entire repository at once to ensure a safe migration. I've noticed that many projects have utility methods around DateTime. If these utility methods aren't migrated, the coverage becomes minimal because Joda-Time types are tightly interconnected. For example, intervals depend on DateTime, DateTime relies on time zones, and this interconnectedness makes everything unsafe if those utility methods are not migrated

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.

2 participants