Skip to content

Conversation

Jammy-Louie
Copy link
Contributor

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

What's changed?

ReplaceStringBuilderWithString will retain the prefix white space for the removed method calls on the argument.

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

Sorry, something went wrong.

@github-project-automation github-project-automation bot moved this to In Progress in OpenRewrite Apr 11, 2025
@timtebeek timtebeek added the bug Something isn't working label Apr 12, 2025
@Jammy-Louie Jammy-Louie changed the title Initial commit with tests ReplaceStringBuilderWithString should keep newlines Apr 14, 2025
@Jammy-Louie Jammy-Louie marked this pull request as ready for review April 14, 2025 13:57
@github-project-automation github-project-automation bot moved this from In Progress to Ready to Review in OpenRewrite Apr 14, 2025
@Laurens-W
Copy link
Contributor

I can't help but wonder if registering a maybeAutoFormat would be preferred here instead of manually messing with the spacing

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Some suggestions could not be made:

  • src/main/java/org/openrewrite/staticanalysis/ReplaceStringBuilderWithString.java
    • lines 32-32

@Jammy-Louie Jammy-Louie force-pushed the issue-88-maintain-newlines-ReplaceStringBuilderWithString branch from a7132e2 to 0f3da5d Compare April 14, 2025 17:54
@timtebeek
Copy link
Member

Nice improvement already! I suggest we merge this once the forced pushed issues are resolved.

Separately after we could look into supporting comments as well, as this right now fails.

    @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" +
                          testString();
                  }
              }
              """
          )
        );
    }

timtebeek and others added 2 commits April 14, 2025 20:15
…derWithString
@timtebeek timtebeek merged commit 5e9b634 into main Apr 14, 2025
2 checks passed
@timtebeek timtebeek deleted the issue-88-maintain-newlines-ReplaceStringBuilderWithString branch April 14, 2025 20:35
@github-project-automation github-project-automation bot moved this from Ready to Review to Done in OpenRewrite Apr 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

ReplaceStringBuilderWithString should keep newlines
4 participants