-
Notifications
You must be signed in to change notification settings - Fork 80
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 recipe RemovedToolProviderConstructor #454
adding recipe RemovedToolProviderConstructor #454
Conversation
src/test/java/org/openrewrite/java/migrate/RemovedToolProviderConstructorTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/migrate/RemovedToolProviderConstructorTest.java
Outdated
Show resolved
Hide resolved
…ConstructorTest.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
src/test/java/org/openrewrite/java/migrate/RemovedToolProviderConstructorTest.java
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/migrate/RemovedToolProviderConstructorTest.java
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/migrate/RemovedToolProviderConstructorTest.java
Outdated
Show resolved
Hide resolved
…ConstructorTest.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…ConstructorTest.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
src/test/java/org/openrewrite/java/migrate/RemovedToolProviderConstructorTest.java
Outdated
Show resolved
Hide resolved
…ConstructorTest.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
src/test/java/org/openrewrite/java/migrate/RemovedToolProviderConstructorTest.java
Show resolved
Hide resolved
…ub.com/ranuradh/rewrite-migrate-java into remove_modifier_bootstrap_toolprovider
src/test/java/org/openrewrite/java/migrate/RemovedToolProviderConstructorTest.java
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/migrate/RemovedToolProviderConstructorTest.java
Show resolved
Hide resolved
@Override | ||
public TreeVisitor<?, ExecutionContext> getVisitor() { | ||
return new JavaVisitor<ExecutionContext>() { | ||
private final MethodMatcher COMPILER_METHODMATCHER = new MethodMatcher("javax.tools.ToolProvider getSystemJavaCompiler()", false); |
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 need to look for all the methods this class contains. We know that every method within the class is static so this recipe just needs to check that the class invoking a method is of type javax.tools.ToolProvider
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.
updated in next commit
|
||
if ( COMPILER_METHODMATCHER.matches(method, false)||(DOCUMENTATION_METHODMATCHER.matches(method, false)) ||(CLASSLOADER_METHODMATCHER.matches(method, false))) { | ||
JavaType.Method transformedType = null; | ||
if (method.getMethodType() != null) { |
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.
Several calls are made to method.getMethodType
. We should go ahead and store the result of that call to a variable so we only invoke that method once.
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.
updated in next commit
|
||
public void test() throws Exception { | ||
ToolProvider tp = null; | ||
ToolProvider.getSystemJavaCompiler(); |
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.
We need to broaden this test case to capture more ways these calls can be make that better illustrate real world scenarios(for example take a look at how ToolProvider.getSystemJavaCompiler()
is called here http://www.java2s.com/example/java-api/javax/tools/toolprovider/getsystemjavacompiler-0-8.html). I think some of the logic in your recipe can be stripped but we need better test coverage to confirm this.
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.
added in the next commit
src/test/java/org/openrewrite/java/migrate/RemovedToolProviderConstructorTest.java
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/migrate/RemovedToolProviderConstructorTest.java
Show resolved
Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
src/test/java/org/openrewrite/java/migrate/RemovedToolProviderConstructorTest.java
Outdated
Show resolved
Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
What's changed?
Created RemovedToolProviderConstructor Recipe:
For the following rule:
What's your motivation?
This recipe checks for a specific method pattern and converts it to a static call.The existing recipe ChangeMethodTargetToStatic did not work in this case since it doesn't meet the criteria of the method should not be static and should not be of the SameReceiverType
Anything in particular you'd like reviewers to focus on?
@cjobinabo
Anyone you would like to review specifically?
Have you considered any alternatives or workarounds?
Any additional context
Checklist