-
Notifications
You must be signed in to change notification settings - Fork 7
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
Inline Javatemplate in RefasterTemplateProcessor #57
Changes from all commits
2f49868
10d7be3
57c8e52
6ce5044
a835294
0ae56a8
dfe8a22
5fb131c
9bf620b
9085931
01cef4b
ef3d59f
4491363
5058d1c
6d449d7
a21d6db
676a188
41e95fb
7cb1bbd
e93e148
f7c954a
f31fb31
a7ac909
71603fc
6f7b6cb
a726122
261ad17
12223ca
0ea5172
50fdf56
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
/* | ||
* Copyright 2023 the original author or authors. | ||
* Copyright 2024 the original author or authors. | ||
* <p> | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
|
@@ -20,11 +20,12 @@ | |
import org.openrewrite.Recipe; | ||
import org.openrewrite.TreeVisitor; | ||
import org.openrewrite.internal.lang.NonNullApi; | ||
import org.openrewrite.java.JavaParser; | ||
import org.openrewrite.java.JavaTemplate; | ||
import org.openrewrite.java.JavaVisitor; | ||
import org.openrewrite.java.search.*; | ||
import org.openrewrite.java.template.Primitive; | ||
import org.openrewrite.java.template.Semantics; | ||
|
||
import org.openrewrite.java.template.function.*; | ||
import org.openrewrite.java.template.internal.AbstractRefasterJavaVisitor; | ||
import org.openrewrite.java.tree.*; | ||
|
@@ -33,12 +34,18 @@ | |
|
||
import static org.openrewrite.java.template.internal.AbstractRefasterJavaVisitor.EmbeddingOption.*; | ||
|
||
|
||
/** | ||
* OpenRewrite recipe created for Refaster template {@code Arrays}. | ||
*/ | ||
@SuppressWarnings("all") | ||
@NonNullApi | ||
public class ArraysRecipe extends Recipe { | ||
|
||
public ArraysRecipe() { | ||
} | ||
/** | ||
* Instantiates a new instance. | ||
*/ | ||
public ArraysRecipe() {} | ||
|
||
@Override | ||
public String getDisplayName() { | ||
|
@@ -53,8 +60,12 @@ public String getDescription() { | |
@Override | ||
public TreeVisitor<?, ExecutionContext> getVisitor() { | ||
JavaVisitor<ExecutionContext> javaVisitor = new AbstractRefasterJavaVisitor() { | ||
final JavaTemplate before = Semantics.expression(this, "before", (String[] strings) -> String.join(", ", strings)).build(); | ||
final JavaTemplate after = Semantics.expression(this, "after", (String[] strings) -> String.join(":", strings)).build(); | ||
final JavaTemplate before = JavaTemplate | ||
.builder("String.join(\", \", #{strings:any(java.lang.String[])})") | ||
.build(); | ||
final JavaTemplate after = JavaTemplate | ||
.builder("String.join(\":\", #{strings:any(java.lang.String[])})") | ||
.build(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The only downside I see with this refactoring is that the generated code is now harder to read and that the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes the main motivator for me was PRs like this one that aim to use Java 9+ constructs in recipes, which previously meant that code was in the generated recipes as well(likely?) bumping the Java version required to run the recipe. With this change I think we should be good to have the generated recipes included with the other Java 8 compatible classes. |
||
|
||
@Override | ||
public J visitMethodInvocation(J.MethodInvocation elem, ExecutionContext ctx) { | ||
|
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.
Surprised the
String
reference isn't qualified here. Will look into that separately.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 see, we have an explicit exception for
java.lang
:rewrite-templating/src/main/java/org/openrewrite/java/template/internal/TemplateCode.java
Line 118 in 261ad17
It does make the template code a bit more readable, but it could cause problems when the Java class an after template gets embedded into is in a package which has a class by the same name, which is unusual but not unheard of.
I am a bit on the fence on this one.
java.lang
is a special case also in theShortenFullyQualifiedTypeReferences
recipe (for basically the same reason). But I think in probably around 90% of the cases the name would end up getting shortened, because the compilation unit already has another (unqualified) reference to the same type in which case the recipe knows it can shorten the reference.As I pointed out before, this isn't directly related to your PR. It is just something that caught my eye.
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.
Yes we can tackle this separately; there's a few other items too I'd been looking to do after this lands, since any changes previously were likely to conflict with this one.