-
Notifications
You must be signed in to change notification settings - Fork 77
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 thread destroy method recipe #246
remove thread destroy method recipe #246
Conversation
For context: https://bugs.openjdk.org/browse/JDK-8204260 |
src/main/java/org/openrewrite/java/migrate/lang/RemoveThreadDestroyMethod.java
Show resolved
Hide resolved
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.
Thanks a lot for implementing this one! Some of the same concerns regarding the use of the type system as in #247 apply here, but I don't think there's anything much we can do about that. Some smaller suggestions that when applied can lead to a merge.
|
||
@Override | ||
public String getDisplayName() { | ||
return "Remove deprecated Thread.destroy() statement"; |
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.
You can use Markdown around code snippets here and in the description such that it'll look slightly better in the documentation.
return "Remove deprecated Thread.destroy() statement"; | |
return "Remove deprecated `Thread.destroy()`"; |
|
||
@Override | ||
public String getDescription() { | ||
return "Remove deprecated invocations of Thread.destroy() which have no alternatives needed."; |
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.
return "Remove deprecated invocations of Thread.destroy() which have no alternatives needed."; | |
return "Remove deprecated invocations of `Thread.destroy()` which have no alternatives needed."; |
|
||
@Override | ||
public void defaults(RecipeSpec spec) { | ||
spec.recipe(new RemoveThreadDestroyMethod()).allSources(s -> s.markers(javaVersion(8))); |
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 there's anything specific around the Java version of the sources here right? Maybe best to then not explicitly set any markers, as it could cause confusion
spec.recipe(new RemoveThreadDestroyMethod()).allSources(s -> s.markers(javaVersion(8))); | |
spec.recipe(new RemoveThreadDestroyMethod()); |
J.MethodInvocation mi = super.visitMethodInvocation(method, ctx); | ||
return (Objects.nonNull(mi.getSelect()) && TypeUtils.isAssignableTo(JAVA_LANG_THREAD, mi.getSelect().getType()) && mi.getSimpleName().equals("destroy")) ? null : mi; |
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.
Suggest to add some newlines here for each of the conditions that we check; and split the ternary into an if statement such that it's clearer that we only make a change (return null
) when those conditions match.
J.MethodInvocation mi = super.visitMethodInvocation(method, ctx); | |
return (Objects.nonNull(mi.getSelect()) && TypeUtils.isAssignableTo(JAVA_LANG_THREAD, mi.getSelect().getType()) && mi.getSimpleName().equals("destroy")) ? null : mi; | |
J.MethodInvocation mi = super.visitMethodInvocation(method, ctx); | |
if (Objects.nonNull(mi.getSelect()) | |
&& TypeUtils.isAssignableTo(JAVA_LANG_THREAD, mi.getSelect().getType()) | |
&& mi.getSimpleName().equals("destroy")) { | |
return null; | |
} | |
return mi; |
import java.lang.*; | ||
|
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.
import java.lang.*; | |
Thank you for your detailed inputs. I've made the necessary changes. Please review them. |
@satvika-eda Thanks for the contribution! |
What's changed?
To remove Thread.destroy deprecated method
What's your motivation?
To automate JDK migration to a greater extent
Checklist
./gradlew licenseFormat