Skip to content

ReplaceStringBuilderWithString should keep comments #511

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

Jammy-Louie
Copy link
Contributor

@Jammy-Louie Jammy-Louie commented Apr 15, 2025

What's changed?

This retains comments that may appear such that the following scenario can be supported.

    @Test
    @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/88")
    void retainComments() {
        rewriteRun(
          //language=java
          java(
            """
              class A {
                  void foo() {
                      String scenarioFour = new StringBuilder()
                          // A
                          .append("A")
                          // B
                          .append("B")
                          // C
                          .append("C")
                          .toString();
                  }
              }
              """,
            """
              class A {
                  void foo() {
                      String scenarioFour =
                          // A
                          "A" +
                          // B
                          "B" +
                          // C
                          "C";
                  }
              }
              """
          )
        );
    }

What's your motivation?

Anything in particular you'd like reviewers to focus on?

Anyone you would like to review specifically?

Have you considered any alternatives or workarounds?

Any additional context

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've read and applied the recipe conventions and best practices
  • I've used the IntelliJ IDEA auto-formatter on affected files

@@ -126,8 +125,7 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu
*/
public static J.Binary concatAdditionBinary(Expression left, Expression right) {
J.Binary b = getAdditiveBinaryTemplate();
String rightWhiteSpace = right.getPrefix().getWhitespace();
Space rightPrefix = rightWhiteSpace.isEmpty() ? Space.SINGLE_SPACE : Space.build(rightWhiteSpace, emptyList());
Space rightPrefix = right.getPrefix().isEmpty() ? Space.SINGLE_SPACE : right.getPrefix();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

avoid building Space so we can preserve Comments

Comment on lines 72 to 80
Expression additive = ChainStringBuilderAppendCalls.additiveExpression(arguments);

if (additive == null) {
return m;
}

if (arguments.get(0).getComments().isEmpty() || !arguments.get(0).getPrefix().getWhitespace().startsWith("\n")) {
additive = additive.withPrefix(method.getPrefix());
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is to ensure no additional white space is added if there are comments and the first argument has a prefix that begins with a new line.

this accounts for cases such as.

    @Test
    @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/88")
    void retainComments() {
        rewriteRun(
          //language=java
          java(
            """
              class A {
                  void foo() {
                      String scenarioOne = new StringBuilder()
                          // A
                          .append("A")
                          // B
                          .append("B")
                          // C
                          .append("C")
                          .toString();
                  }
              }
              """,
            """
              class A {
                  void foo() {
                      String scenarioOne =
                          // A
                          "A" +
                          // B
                          "B" +
                          // C
                          "C";
                  }
              """
          )
        );
    }

@@ -111,7 +120,7 @@ private List<Expression> adjustExpressions(J.MethodInvocation method, List<Expre
}
}
} else if (!(arg instanceof J.Identifier || arg instanceof J.Literal || arg instanceof J.MethodInvocation)) {
return new J.Parentheses<>(randomId(), Space.EMPTY, Markers.EMPTY, JRightPadded.build(arg));
return new J.Parentheses<>(randomId(), arg.getPrefix(), Markers.EMPTY, JRightPadded.build(arg.withPrefix(Space.EMPTY)));
Copy link
Contributor Author

@Jammy-Louie Jammy-Louie Apr 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

preserve the prefix on the Parentheses

so if the comments would look like

// comments
(1 + 1)

instead of

(
// comments
1 + 1)

@Jammy-Louie Jammy-Louie marked this pull request as ready for review April 15, 2025 18:19
@timtebeek timtebeek moved this from In Progress to Ready to Review in OpenRewrite Apr 15, 2025
Copy link
Member

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix!

@timtebeek timtebeek merged commit 03387d4 into main Apr 15, 2025
2 checks passed
@timtebeek timtebeek deleted the issue-88-maintaiin-comments-ReplaceStringBuilderWithString branch April 15, 2025 19:19
@github-project-automation github-project-automation bot moved this from Ready to Review to Done in OpenRewrite Apr 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants