Skip to content
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

indent step not playing nicely with multiline string literals #1628

Open
4 of 6 tasks
bmarwell opened this issue Mar 15, 2023 · 8 comments
Open
4 of 6 tasks

indent step not playing nicely with multiline string literals #1628

bmarwell opened this issue Mar 15, 2023 · 8 comments
Labels

Comments

@bmarwell
Copy link

bmarwell commented Mar 15, 2023

If you are submitting a bug, please include the following:

  • summary of problem
  • gradle or maven version
  • spotless version
  • operating system and version
  • copy-paste your full Spotless configuration block(s), and a link to a public git repo that reproduces the problem if possible
  • copy-paste the full content of any console errors emitted by gradlew spotless[Apply/Check] --stacktrace

setup

Maven any 3.8.x

spotless 2.32.0

OS - Mac, Linux, windows

What happened?

Reformats and destroys text blocks

source:

class TestClas {
  @Test
  public void serverFilecheck() {
    assertThat(myFile())
        .as("file [%s] any line contains [limitFieldSize=\"8192\"].", myFile())
        .content(StandardCharsets.UTF_8)
        .contains(
            """
  <httpEndpoint
    id="defaultHttpEndpoint"
    host="localhost"
    httpPort="8080"
    limitFieldSize="8192"
""");
  }
}

reformatted:

class TestClas {
  @Test
  public void serverFilecheck() {
    assertThat(myFile())
        .as("file [%s] any line contains [limitFieldSize=\"8192\"].", myFile())
        .content(StandardCharsets.UTF_8)
        .contains(
            """
<httpEndpoint
id="defaultHttpEndpoint"
host="localhost"
httpPort="8080"
limitFieldSize="8192"
""");
  }
}

Config:

spotless + indent 4 tabs + then indent 2 spaces (recommended workaround by spotless to have palantir with two spaces)

      <plugin>
        <groupId>com.diffplug.spotless</groupId>
        <artifactId>spotless-maven-plugin</artifactId>
        <version>2.32.0</version>
        <inherited>true</inherited>
        <configuration>
          <java>
            <toggleOffOn/>
            <palantirJavaFormat>
              <version>2.28.0</version>
            </palantirJavaFormat>

            <importOrder/>
            <removeUnusedImports/>
            <trimTrailingWhitespace/>
            <endWithNewline/>
            <formatAnnotations/>

            <indent>
              <tabs>true</tabs>
              <spacesPerTab>4</spacesPerTab>
            </indent>
            <indent>
              <spaces>true</spaces>
              <spacesPerTab>2</spacesPerTab>
            </indent
          </java>

          <pom>
            <includes>
              <include>pom.xml</include>
            </includes>

            <sortPom>
              <expandEmptyElements>false</expandEmptyElements>
            </sortPom>
          </pom>
        </configuration>
        <executions>
          <execution>
            <goals>
              <goal>check</goal>
            </goals>
            <phase>validate</phase>
          </execution>
        </executions>
      </plugin>

What did you want to happen?

Maybe nudge the text block to the right, but the difference between the closing """ and the text MAY NOT BE ALTERED!

@bmarwell
Copy link
Author

It even reformats the code with // spotless:off 😞

@bmarwell bmarwell changed the title [BUG] Reformats and destroys text blocks [BUG] Reformats then reindent destroys text blocks Mar 15, 2023
@bmarwell bmarwell changed the title [BUG] Reformats then reindent destroys text blocks [BUG] applying palantir then reindent destroys text blocks Mar 15, 2023
@bmarwell
Copy link
Author

Any workarounds appreciated!

@nedtwigg
Copy link
Member

the difference between the closing """ and the text MAY NOT BE ALTERED!

It looks like the "source" and "reformatted" that you copy-pasted above are the same. Is that a mistake? Regardless, I would try commenting out a step until you know exactly which step is causing this issue. I would guess it is palantir. Then I would file an issue in their bugtracker.

It even reformats the code with // spotless:off 😞

For sure you can workaround this bug with spotless:off. Do you have a tag // spotless:off and a matching //spotless:on afterwards? You need both.

@bmarwell
Copy link
Author

Hey!
👋🏻

No it's different, see the text block! It modified the Text block. Look closely.

Yes, I tried and it's NOT palantir. Only if I add the indent steps. Oh, and yes, I had both comments in there and it didn't work. Will try to set to a demo project on the train tomorrow.

@nedtwigg
Copy link
Member

Ahhh, I see it now. You are correct, this is a problem in our indent step.

BEFORE
  <httpEndpoint
    id="defaultHttpEndpoint"
AFTER
<httpEndpoint
id="defaultHttpEndpoint"

We're turning every 4 spaces into a tab, and then turning each tab into 2 spaces. The indent step just looks for leading spaces, it doesn't (currently) know anything about the syntax of multiline string literals on the various languages it is used for.

So the first line, indented by 2 spaces, which is not a multiple of 4, it makes sense that it gets clobbered. But I would expect the second line to be okay. We picked bad names for these steps, and we should fix that in our next breaking version change

And regardless of that, spotless:off should definitely be able to fix this. I can see three different things going wrong here

  1. spotles:off should be able to fix this
  2. the second line should be okay
  3. the first line "should" be broken, which shows that we need to rename these indent steps better, and possibly also make them language-aware so they understand multiline string literals.

@nedtwigg nedtwigg changed the title [BUG] applying palantir then reindent destroys text blocks indent step not playing nicely with multiline string literals Mar 20, 2023
@bmarwell
Copy link
Author

bmarwell commented Mar 21, 2023

100% agree! For now, the quickest and easiest solution would probably be to get the off working. I'm traveling right now, so I'm terribly sorry I wasn't able to upload a reproducer.

@bmarwell
Copy link
Author

@nedtwigg are we still waiting for palantir/palantir-java-format#930 ?

@rohan-changejar
Copy link

@bmarwell thanks for the duplicate tag, any way forward on this topic?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants