Discussion/question: Is the default application of UseTextBlocks too 'opinionated' ?

[jjank]

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 👍

[timtebeek]

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]

but people can still override the default to get even more text blocks. Would that be acceptable here?

In my opinion: yes. Giving users a way to easily "opt-out" would be a good compromise 👍

[timtebeek]

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]

And of course as always: let me know if you'd like to help out either with this one or #261 😇

[yeikel]

For what it is worth, I think that not using TextBlocks in favor of the concatenation is a code smell

[timtebeek]

We'll continue to look for a proper fix in #261 , as discussed above.