UseMavenCompilerPluginReleaseConfiguration does not respect existing indentation

Philzen commented 8 months ago

What version of OpenRewrite are you using?

I am using OpenRewrite v5.10.0 with rewrite-migrate-java:2.2.0

How are you running OpenRewrite?

Using the CLI command from https://docs.openrewrite.org/recipes/java/migrate/upgradetojava17:

mvn -U org.openrewrite.maven:rewrite-maven-plugin:run \
  -Drewrite.recipeArtifactCoordinates=org.openrewrite.recipe:rewrite-migrate-java:RELEASE \
  -Drewrite.activeRecipes=org.openrewrite.java.migrate.UpgradeToJava17

What is the smallest, simplest way to reproduce the problem?

Have a POM.xml with 4 spaces of indentation, such as this (only showing relevant section)

            <plugin>
                <artifactId>maven-compiler-plugin</artifactId>
                <configuration>
                    <source>${java.version}</source>
                    <target>${java.version}</target>
                    <compilerArgument>-Xlint:all</compilerArgument>
                    <failOnWarning>true</failOnWarning>
                </configuration>
            </plugin>

What did you expect to see?

diff --git a/pom.xml b/pom.xml
index cef0ec1b..1212ca09 100644
--- a/pom.xml
+++ b/pom.xml
@@ -41,12 +41,11 @@
             </plugin>
             <plugin>
                 <artifactId>maven-compiler-plugin</artifactId>
-                <configuration>
-                    <source>${java.version}</source>
-                    <target>${java.version}</target>
-                    <compilerArgument>-Xlint:all</compilerArgument>
-                    <failOnWarning>true</failOnWarning>
+                <configuration>
+                    <release>${java.version}</release>
+                    <compilerArgument>-Xlint:all</compilerArgument>
+                    <failOnWarning>true</failOnWarning>
                 </configuration>
             </plugin>
             <plugin>
@@ -280,9 +279,8 @@

What did you see instead?

diff --git a/pom.xml b/pom.xml
index cef0ec1b..1212ca09 100644
--- a/pom.xml
+++ b/pom.xml
@@ -41,12 +41,11 @@
             </plugin>
             <plugin>
                 <artifactId>maven-compiler-plugin</artifactId>
-                <configuration>
-                    <source>${java.version}</source>
-                    <target>${java.version}</target>
-                    <compilerArgument>-Xlint:all</compilerArgument>
-                    <failOnWarning>true</failOnWarning>
+        <configuration>
+          <compilerArgument>-Xlint:all</compilerArgument>
+          <failOnWarning>true</failOnWarning>
+          <release>${java.version}</release>
                 </configuration>
             </plugin>
             <plugin>

Actually, these are two things here:

the indentation (obviously)
the insertion is not done in place – it would be ideal if <release> would be inserted at exactly the same position where it replaces <target> and <source>

timtebeek commented 4 months ago

Thanks for the report! Not sure if you recall at this point, but was formatting otherwise consistent in that file and similar files across the project? We try to detect the style used and apply based on that, which could be thrown off if the majority of files use a different style of indentation.

Philzen commented 4 months ago

@timtebeek Yes, the project very likely contained a mix of files containing tab and space indentations. Unfortunately i have no statistics the distribution of different styles in different file types in that project. Seems similar to issue https://github.com/openrewrite/rewrite-testing-frameworks/issues/412

However, the concerned file in this report example did have consistent space indentation. I would have expected the mimium requirement of an autoformatter not to change the style withing a file. Ideally it would respect .editorconfig (which the project had) or other formatting configuration, such as a checkstyle config XML.

openrewrite / rewrite