Repository navigation
Discussion/question: Is the default application of UseTextBlocks too 'opinionated' ? #260
Description
Hi there,
this is not a bug, merely a question / point of discussion:
While executing a Spring Boot 3.1 migration (from 3.0.8) I noticed that UseTextBlocks did a couple of changes:
$ mvn -U org.openrewrite.maven:rewrite-maven-plugin:run \
-Drewrite.recipeArtifactCoordinates=org.openrewrite.recipe:rewrite-spring:RELEASE \
-Drewrite.activeRecipes=org.openrewrite.java.spring.boot3.UpgradeSpringBoot_3_1
Changes have been made to ****.java by:
org.openrewrite.java.spring.boot3.UpgradeSpringBoot_3_1
org.openrewrite.java.spring.boot3.UpgradeSpringBoot_3_0
org.openrewrite.java.migrate.UpgradeToJava17
org.openrewrite.java.migrate.lang.UseTextBlocks: {convertStringsWithoutNewlines=true}
(version was rewrite:5.3.1)
The diff looks a little bit...unfortunate in my opinion:
My question is: Is the default inclusion of org.openrewrite.java.migrate.lang.UseTextBlocks in org.openrewrite.java.migrate.UpgradeToJava17 really desirable? Because technically nothing forces developers too use text blocks (don't get me wrong, I love them). It's a 'stylistic' change but nothing 'functional'.
I do realize that this also comes down to personal preference but my personal opinion is that it introduces changes that might not be desirable in every situation.
Thanks for your thoughts about this 👍
Activity
timtebeek commentedon Jul 18, 2023
Thanks for bringing up this concern @jjank ! It's indeed a known limitation of the recipe (#261) which we still hope to resolve such that it does not cause any issues for our users.
I do agree that perhaps we should not be making changes if we recognize such a case, such that we can still add in some text blocks, but make it an optional (default false) to also add such known-incomplete text blocks. Then when we include the recipe we have that set to the default, but people can still override the default to get even more text blocks. Would that be acceptable here?
Alternatively we could try to write a recipe that first turns such string concatenation into a formatted String, which would then be easier to convert into a text block. But that might also have some challenges to get right.
jjank commentedon Jul 18, 2023
In my opinion: yes. Giving users a way to easily "opt-out" would be a good compromise 👍
timtebeek commentedon Jul 18, 2023
Then let's have this ticket be the focus point of adding such an optional back-off, whereas we keep #261 with a focus on improving the recipe itself, such that a back-off option is not necessary as often.
timtebeek commentedon Jul 18, 2023
And of course as always: let me know if you'd like to help out either with this one or #261 😇
yeikel commentedon Aug 7, 2023
For what it is worth, I think that not using TextBlocks in favor of the concatenation is a code smell
Do not partially use text blocks
Do not partially use text blocks (#273)
timtebeek commentedon Aug 16, 2023
We'll continue to look for a proper fix in #261 , as discussed above.