-
Notifications
You must be signed in to change notification settings - Fork 109
Remove limitations of recipe for making safe migration #810
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 limitations of recipe for making safe migration #810
Conversation
src/main/java/org/openrewrite/java/migrate/joda/JodaTimeVisitor.java
Outdated
Show resolved
Hide resolved
| return original; // unhandled case | ||
| } | ||
| if (template.getTemplate().getCode().equals(JODA_MULTIPLE_MAPPING_POSSIBLE)) { | ||
| System.out.println(JODA_MULTIPLE_MAPPING_POSSIBLE + ": " + original); |
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 don't think we want to report anything via stdout.
If anything, for such a cases we place some Marker and attach to the source LST element. I am not fully if that's necessary in this specific case, maybe just refrain from making any change? I am not sure.
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.
The idea is to report on missing mapping. It was extremely useful for me during development. And can be used for reporting later.
…r.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
Do we still need these changes following this merged PR ? |
Yes. It is still valid. I don't know if your question is relevant anymore, as you reverted that merge. |
|
Hello |
|
As discussed, we'll continue development in https://github.com/openrewrite/rewrite-joda Thanks again! |
What's changed?
Joda recipe can perform a safe migration.
What's your motivation?
To execute a fully automated migration
Anything in particular you'd like reviewers to focus on?
Remove of check for
unsafeVarsAnyone you would like to review specifically?
@amishra-u @timtebeek
Any additional context
With this change, tests for Scanner are obsolete.
I am not even sure we need
ScanningRecipeanymoreChecklist