-
Notifications
You must be signed in to change notification settings - Fork 8
Added implementation for classpathFromResource in Refaster templates #139
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
base: main
Are you sure you want to change the base?
Added implementation for classpathFromResource in Refaster templates #139
Conversation
builder.append("\n .javaParser(JavaParser.fromJavaVersion().classpath(JavaParser.runtimeClasspath()))"); | ||
// builder.append("\n .javaParser(JavaParser.fromJavaVersion().classpath(JavaParser.runtimeClasspath()))"); | ||
builder.append("\n .javaParser(JavaParser.fromJavaVersion().classpathFromResources(ctx, "); | ||
builder.append(jarNames.stream().map(jarName -> '"' + jarName + '"').collect(joining(", "))); |
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.
jarNames
is constructed from ClasspathJarNameDetector
. Do we need to check if the resulting list is correct and complete, including dependencies from type tables?
@@ -95,13 +95,13 @@ public J visitMethodInvocation(J.MethodInvocation elem, ExecutionContext ctx) { | |||
JavaTemplate.Matcher matcher; | |||
if ((matcher = JavaTemplate | |||
.builder("String.format(\"\\\"%s\\\"\", com.sun.tools.javac.util.Convert.quote(#{value:any(java.lang.String)}))") | |||
.javaParser(JavaParser.fromJavaVersion().classpath(JavaParser.runtimeClasspath())) | |||
.javaParser(JavaParser.fromJavaVersion().classpathFromResources(ctx, "tools")) |
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.
How do we "know" that tools is in fact on the classpath or in a type table?
How do we handle situations where the names listed do not match the class path/typetables?
There are several tests for |
String joinedJarNames = jarNames.stream().collect(joining(", ", "\"", "\"")); | ||
builder.append("\n .javaParser(JavaParser.fromJavaVersion().classpathFromResources(ctx, ") | ||
.append(joinedJarNames) | ||
.append("))\n "); |
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.
Hmm; this now breaks the tests we have for the TemplateProcessor, as the ctx
can not be found there, and it looks like that might not be easy to introduce where called from openrewrite/rewrite.
public class ParameterReuseRecipe { | |
JavaIsoVisitor visitor = new JavaIsoVisitor<ExecutionContext>() { | |
JavaTemplate.Builder before = Semantics.expression(this, "before", (String s) -> s.equals(s)); | |
JavaTemplate.Builder after = Semantics.expression(this, "after", (String s) -> true); | |
}; | |
} |
Luckily it's not a feature we use often, but still it'd be a shame to break that here.
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 do use it in rewrite-micrometer, IIRC.
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.
There's indeed three places:
- https://github.com/openrewrite/rewrite-apache/blob/df33d276534aaaea4c2f2f93ad681954084e09b2/src/main/java/org/openrewrite/apache/commons/lang/IsNotEmptyToJdk.java#L74-L77
- https://github.com/openrewrite/rewrite-migrate-java/blob/6b98403236ad24c7d949750cd1c7af5fb94eebd8/src/main/java/org/openrewrite/java/migrate/UseJavaUtilBase64.java#L126-L127
- https://github.com/openrewrite/rewrite-micrometer/blob/277c471b5d22db127cd86f00eff7e8ac933c8c9b/src/main/java/org/openrewrite/micrometer/misk/MigrateEmptyLabelMiskCounter.java#L71-L73
What's changed?
Changed implementation in TemplateCode when generating refaster recipes to use classPathFromResource
What's your motivation?
Ensures Refaster template based recipes are able to match and use external types when running outside of build plugins.