Formatting in @AfterTemplate is ignored, leading to huge single-line statements after the migration

[Philzen]

What version of OpenRewrite are you using?

I am using

• OpenRewrite 8.27.1
• rewrite-templating:1.9.2
• error_prone_core:2.28.0

I have the following refaster recipe:

    public static class MigrateAssertEqualsNoOrderIteratorWithMessage {

        @BeforeTemplate void before(Iterator<?> actual, Iterator<?> expected, String message) {
            Assert.assertEqualsNoOrder(actual, expected, message);
        }

        @AfterTemplate void after(Iterator<?> actual, Iterator<?> expected, String message) {
            Assertions.assertArrayEquals(
                StreamSupport.stream(Spliterators.spliteratorUnknownSize(expected, 0), false).sorted().toArray(),
                StreamSupport.stream(Spliterators.spliteratorUnknownSize(actual, 0), false).sorted().toArray(),
                message
            );
        }
    }

(which is already breaking the 100 character barrier, but gives a better trade-off regarding cognitive complexity)

What did you expect to see?

The migrated code should keep the formatting.

What did you see instead?

It creates this monster:

    Assertions.assertArrayEquals(StreamSupport.stream(Spliterators.spliteratorUnknownSize(expected, 0), false).sorted().toArray(), StreamSupport.stream(Spliterators.spliteratorUnknownSize(actual, 0), false).sorted().toArray(), "Should contain the same elements");

I can see that the generated code has ignored the formatting, the JavaTemplate string has no line breaks or indents whatsoever:

    @NonNullApi
    public static class MigrateAssertEqualsNoOrderIteratorWithMessageRecipe extends Recipe {
    // …
                final JavaTemplate after = JavaTemplate
                        .builder("org.junit.jupiter.api.Assertions.assertArrayEquals(java.util.stream.StreamSupport.stream(java.util.Spliterators.spliteratorUnknownSize(#{expected:any(java.util.Iterator<?>)}, 0), false).sorted().toArray(), java.util.stream.StreamSupport.stream(java.util.Spliterators.spliteratorUnknownSize(#{actual:any(java.util.Iterator<?>)}, 0), false).sorted().toArray(), #{message:any(java.lang.String)});")
                        .javaParser(JavaParser.fromJavaVersion().classpath(JavaParser.runtimeClasspath()))
                        .build();
    // …

[timtebeek]

Thanks for the detailed look & report! I'm not sure if the RefasterTemplateProcessor would even have access to any formatting or comments (which came up before), but we can certainly strive to do a better job at formatting than placing everything on a single line. Perhaps leveraging auto format in the generated recipes could be of help, but I'm not sure what that would do for performance, or even have the desired effect.

[knutwannheden]

Yes, we could certainly add formatting as a default embedding option. That should be easy to implement. Alternatively, maintain the formatting from the template might also be possible. But that would be more work.

[Philzen]

Alternatively, maintain the formatting from the template might also be possible.

This is actually what i'm asking for here. The template string in the generated JavaTemplate.builder should equal what is defined in the @AfterTemplate – otherwise DX may be suboptimal, because the accompanying test one writes may fail with a result that looks non-deterministic to recipe developers.

IMHO a formatting run should be a separate step. However if that's the way to go it'd be good to have some determinism as a recipe developer, so one can anticipate the result to put into the tests.

[Philzen]

Yes, we could certainly add formatting as a default embedding option.

@knutwannheden There already seems to be some kind of auto-formatting at work, i.e. when using statement lambdas instead of expression lambdas – and the mileage of it varies.

---

Here is an example where it is really helpful as there is literally no way define that kind of formatting as part of the template:

@AfterTemplate void after(Class<? extends Throwable> throwableClass, Executable executable) {
    Assertions.assertThrows(throwableClass, executable);
}

… will lead to this migration:

@Test void runnableWithNoExpectedType() {
   // language=java
    rewriteRun(java(
        """
        import org.testng.Assert;
        
        class MyTest {
            void testMethod() {
                Assert.assertThrows(() -> { throw new RuntimeException(); });
            }
        }
        """,
        """
        import org.junit.jupiter.api.Assertions;
        
        class MyTest {
            void testMethod() {
                Assertions.assertThrows(Throwable.class, () -> {
                    throw new RuntimeException();
                });
            }
        }
        """
   ));
}

Neat.

---

But over here is an example where it very much gets into the way. In that example it becomes really hard to tell that there are two arguments passed to the method as the end of the first one and the beginning of the second one are put on the same line.

---

So again, my suggestion would be to aim at preserving comments and formatting from the @AfterTemplate definition as close as possible and not to apply any kind of auto-formatting.

I was indeed already contemplating if i should post a new issue with an RFC to implement a new annotation for Refaster templates that would enable to configure exactly that, which i would want to add to the refaster template in the first example here.