Minor issue with StringFormatted recipe

motlin commented 3 months ago

What version of OpenRewrite are you using?

I am using

Maven/Gradle plugin v5.26.0
rewrite-migrate-java v2.11.0

How are you running OpenRewrite?

I am using the Maven plugin, and my project is a multi module project.

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

Run org.openrewrite.java.migrate.lang.StringFormatted on a String.format() call that has a newline after the open paren:

String error = String.format(
        "Overwrote system property {%s:%s} with %s.",
        key,
        oldValue,
        value);

What did you expect to see?

Rewritten code like this:

String error =
        "Overwrote system property {%s:%s} with %s.".formatted(
        key,
        oldValue,
        value);

Or like this:

String error = "Overwrote system property {%s:%s} with %s.".formatted(
        key,
        oldValue,
        value);

What did you see instead?

Code like the first example above, but there's a trailing space character after the equals sign.

String error = // <-- extra, trailing space here
        "Overwrote system property {%s:%s} with %s.".formatted(
        key,
        oldValue,
        value);

This is a minor inconvenience, but Checkstyle immediately fails on the rewritten code.

timtebeek commented 3 months ago

Thanks for the clear and detailed report @motlin ; Such a space is stored in a prefix of the next LST element, and it appears we have no explicit handling of the prefix in the current implementation.

https://github.com/openrewrite/rewrite-migrate-java/blob/cca0195e404553416301aa60064374266002a223/src/main/java/org/openrewrite/java/migrate/lang/StringFormatted.java#L77-L84

Typically you'd see a .withPrefix call that could remove any prefix if no longer needed.

If you want to help resolve this issue the best place to start is with a reproduction test similar to this one, and from there make the required change: https://github.com/openrewrite/rewrite-migrate-java/blob/cca0195e404553416301aa60064374266002a223/src/test/java/org/openrewrite/java/migrate/lang/StringFormattedTest.java#L35-L64

Hope that helps!

bsmahi commented 2 months ago

Hi @timtebeek have tried with the mi = mi.withPrefix(Space.EMPTY); written test case for the above scenarios, it is working fine however, we need to fix remaining text cases as well which are failing

Expression select = wrapperNotNeeded ? arguments.get(0) :
                new J.Parentheses<>(randomId(), Space.EMPTY, Markers.EMPTY, JRightPadded.build(arguments.get(0)));
  mi = mi.withSelect(select);
  //mi = mi.withPrefix(Space.SINGLE_SPACE);
  mi = mi.withPrefix(Space.EMPTY);
  mi = mi.withArguments(arguments.subList(1, arguments.size()));
  if(mi.getArguments().isEmpty()) {
        // To store spaces between the parenthesis of a method invocation argument list
        // Ensures formatting recipes chained together with this one will still work as expected
       mi = mi.withArguments(singletonList(new J.Empty(randomId(), Space.EMPTY, Markers.EMPTY)));
  }

Please let me know your thoughts?

Thanks, Mahi

timtebeek commented 2 months ago

hi @bsmahi ! Thanks for helping replicate this issue! It would be most helpful if you could open a draft pull request from a fork of rewrite-migrate-java with the tests you've added, and the code change proposal above. That way I check out your proposed changes in my IDE for review.

openrewrite / rewrite-migrate-java