-
Notifications
You must be signed in to change notification settings - Fork 349
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
Isolate JavaTemplate issue with generic type parameters #4372
Isolate JavaTemplate issue with generic type parameters #4372
Conversation
rewrite-java-test/src/test/java/org/openrewrite/java/JavaTemplateTest.java
Outdated
Show resolved
Hide resolved
For context: the generated template in this case is import batch.StepBuilder;
import org.openrewrite.java.internal.template.__M__;
import org.openrewrite.java.internal.template.__P__;
class Foo{void test(){
/*__TEMPLATE__*/new StepBuilder()/*__TEMPLATE_STOP__*/
./*__TEMPLATE_STOP__*/<String>method();}} |
As this trips up the auto format in IntelliJ
if (method.getTypeParameters() != null) { | ||
// For method chains return `select` if `STOP_COMMENT` is found before `typeParameters` | ||
if (stopCommentExists(mi.getPadding().getTypeParameters().getBefore().getComments())) { | ||
//noinspection ConstantConditions | ||
return mi.getSelect(); | ||
} | ||
} |
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.
This is what fixed the unit test added here, for the minimal possible impact change we could do here.
A larger change could reduce remove the __TEMPLATE_STOP__
added twice, but it's unclear what unintended side effects that might have downstream, so I went for this safer option.
private void template(Cursor cursor, J prior, StringBuilder before, StringBuilder after, J insertionPoint, JavaCoordinates.Mode mode) { | ||
if (contextSensitive) { | ||
contextTemplate(cursor, prior, before, after, insertionPoint, mode); | ||
} else { | ||
contextFreeTemplate(cursor, prior, before, after); | ||
} | ||
} |
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.
Inlined above for simplicity, as this was only used once and gave a confusion indirection with unused arguments.
@@ -174,7 +172,8 @@ private static String printTreeElement(Tree tree) { | |||
return s != null ? s : ""; | |||
} | |||
|
|||
String[] lines = tree.toString().split("\n"); | |||
String precedingComments = tree instanceof J ? printComments(((J) tree).getPrefix().getComments()) : ""; |
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.
Discovered while debugging the below; figured helpful for folks to see preceding comments too.
Merging already given how this is guarded to only affect cases that would have previously failed, to continue work on |
What's changed?
Replicate generic type issue affecting a downstream PR.
What's your motivation?
This is holding up
Anything in particular you'd like reviewers to focus on?
Not yet explored the fix; figured log the issue first to document the problem in isolation. Any help welcome.
Anyone you would like to review specifically?
@knutwannheden or @sambsnyd