Repository navigation
Recipe UseTextBlocks: Textblock not constructed AFTER it encounters a variable in a string. #261
Description
Was testing with the UseTextBlocks recipe. Wanted to use it for our SQL statements. Noticed that it works partially. Our SQL statement consists out of + variable or constant + . It appears that after encountering a variable a textblock is no longer created. Also reran mvn rewrite:run again just to see if it would be picked up in a second run. It doesn't. During the test the ONLY active recipe was UseTextBlocks
Question: Is there a minimum number of string concats needed before a textblock is used??
Gonna check all the converted files, should I find more 'interesting things' I can enter it here in this thread.
Before
int thisVariableStopsTextBlocks = 1;
String stringWithVariableInIt =
"This " +
"is " +
"text " +
"BEFORE the variable " +
thisVariableStopsTextBlocks +
"This " +
"is " +
"text " +
"AFTER the variable. " +
"As is visible a TextBlock is no longer constructed";
After org.openrewrite.java.migrate.lang.UseTextBlocks: {convertStringsWithoutNewlines=true}
int thisVariableStopsTextBlocks = 1;
String stringWithVariableInIt =
"""
This \
is \
text \
BEFORE the variable \
""" +
thisVariableStopsTextBlocks +
"This " +
"is " +
"text " +
"AFTER the variable. " +
"As is visible a TextBlock is no longer constructed";
Metadata
Metadata
Assignees
Labels
Type
Projects
- Status
Activity
timtebeek commentedon Apr 2, 2023
I'm not immediately seeing a minimum number of lines in UseTextBlocks, nor an explicit test to verify such behavior in UseTextBlocksTest. It appears to look for string concatenations on new lines. Does that answer your question?
jbessels commentedon Apr 2, 2023
Tim thanks for the effort, appreciated. I currently have an 'ExamplesForOpenRewrite' class for other tickets I created. Was to lazy for the first ticket of the day......Have played a bit with TextBlocks. Looks what you say is true. They must be on separate lines. I Can NOT force it with \n in a string.
Before
After
kunli2 commentedon Apr 3, 2023
Hi @jbessels , Thanks for reporting this, yeah, for the question
Question: Is there a minimum number of string concatenation needed before a text block is used?, @timtebeek answered it, there is no minimum number of string concatenation needed but new line/lines are required in string concatenation.This is a known unsupported string grouping now, it will be good to have an enhancement for this.
jbessels commentedon Apr 3, 2023
Tbh a wee bit bit sad that the ticket degraded from 'bug' to enhancement' but I fully understand. Bigger fish to fry first. I've some to offer ;-) It doesn't happen in that many places. I guess I can put the existing contcats part after the ' breaking variable' in a variable itself, rerun UseTextBlocks and have a new TextBlock. remove the variable and then concat it after the 'breaking variable.'
kunli2 commentedon Apr 3, 2023
Yeah, that works, but it will be easier for you if we can support grouping, I think I can do this enhancement in two weeks (probably this week), so you don't need to change the code. :)
jbessels commentedon Apr 3, 2023
I think I can restrain myself for that long ;-) Thanks for the heads up, had almost started with the 'manual labor' task. Your very fast response is very much appreciated!!
jbessels commentedon Apr 11, 2023
Any news on the 'grouping' improvement??
kunli2 commentedon Apr 11, 2023
Hi @jbessels, our team just prioritized the tasks, and we are planning to work on some high-priority tasks at this moment.
and of course, we always welcome a PR implementing from the community.
jbessels commentedon Apr 11, 2023
No problem and I fully understand. So far (including this issue) I've had very good support from Moderne and its all very much appreciated. Then I'm just going for my original solution, some manual labor. As said before: I guess I can put the existing contcats part after the ' breaking variable' in a variable itself, rerun UseTextBlocks and have a new TextBlock. remove the variable and then concat it after the 'breaking variable.'
kunli2 commentedon Apr 12, 2023
Yes, I think it works.
timtebeek commentedon Aug 16, 2023
As of #273 we no longer create partial text blocks; we can track creating either multiple text blocks, or switching over to a single text block with for instance """.formatted here.
jbessels commentedon Aug 29, 2023
I tested the tmp fix (#273) and AFAICT it works.
I REALLY hope a real fix like "switching over to a single text block with for instance """.formatted is going to be implemented as we have LOTS of constructions as mentioned in the OP. Now we currently end up with lots of classes where textblocks and the string concats reside in one file, which is suboptimal.....