Including StringFormatted as part of UpgradeToJava17 seems too opinionated

[jpraet]

Should StringFormatted really be part of org.openrewrite.java.migrate.UpgradeToJava17?

It feels a bit too opinionated to me. String.format() isn't deprecated, and replacing it by "".formatted() doesn't always lead to more readable code.

Given that it's basically impossible to exclude a single recipe, maybe it could be removed from UpgradeToJava17?

[timtebeek]

Hi @jpraet ; I wouldn't say it's impossible; quite the opposite: we've made it easy to tailor the recipes to your needs with the builder, and then share your recipe internally using our rewrite-recipe-starter. That's a well established pattern that we think works well.

I do understand that it can take some getting used to with the .formatted() style, but I do wonder what the problematic cases are that you're referring to. Perhaps we can tweak the recipe not to make any change for convincing cases? That's always our preferred approach.

[jpraet]

Hi @jpraet ; I wouldn't say it's impossible; quite the opposite: we've made it easy to tailor the recipes to your needs with the builder, and then share your recipe internally using our rewrite-recipe-starter. That's a well established pattern that we think works well.

I should have said basically impossible without duplicating the recipe collection and removing the ones I don't want.
Which as I understand it is simplified by the builder, but in the end it's basically still copy/paste right?
If something changes to the upstream recipe collection I copied, my "fork" of that recipe will not be updated?

An example of what throws me off in the .formatted() style is when it needs to add parenthesis around a concatenated string.

More generally to me this feels out of scope of a general purpose "Upgrade to JDK17" recipe, as it is a stylistic change with no other driving force behind it like deprecation or a clear guideline.

rewrite-migrate-java/src/main/resources/META-INF/rewrite/java-version-17.yml

Lines 20 to 25 in 05b8264

	description: >-
	This recipe will apply changes commonly needed when migrating to Java 17. Specifically, for those
	applications that are built on Java 8, this recipe will update and add dependencies on J2EE libraries that are no
	longer directly bundled with the JDK. This recipe will also replace deprecated API with equivalents when there is a
	clear migration strategy. Build files will also be updated to use Java 17 as the target/source and plugins will be
	also be upgraded to versions that are compatible with Java 17.

Anyway, I guess it's a matter of preference. I just wanted to open this discussion.

[timtebeek]

Thanks for that context @jpraet ; I think this PR strikes the right balance then:

• StringFormatted should not wrap first argument by default in Java 17 upgrade #618

[timtebeek]

And know that you feedback is appreciated; we won't always agree on preferences, but where possible we do try to strike that right balance. We've gone through similar changes before with the introduction of text blocks, and I've copied that approach here.

More generally: from time to time we get requests to minimize the changes made by recipes. Some folks would similarly only want to be runtime compatible with Java 21 for instance, and forego the SequencedCollection recipes that introduce getFirst() and getLast(). We think including those changes helps the code quality overall; and the builder is then a way for folks to diverge when they want to.

Your case above was convincing enough to make changes; if you have any more feel free to raise those here, or in a draft unit test to explain what you're after.