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 rule wants to add trailing newlines/whitespaces to multi-line raw string literals #1375

Closed
RBusarow opened this issue Feb 20, 2022 · 8 comments · Fixed by #1401
Closed

Comments

@RBusarow
Copy link

Expected Behavior

The indent rule should not change the values of raw strings.

Observed Behavior

Since 0.44.0 (specifically from PR #1262), the indent rule changes the position of the closing quotes of any multi-line raw string literal so that they're aligned. This isn't just a formatting change, since the added spaces and/or newline are added to the value of the actual string.

Steps to Reproduce

Given this top-level (top level because the indent matters) property:

val someCodeBlock = """
  foo()"""

Formatting will change it to:

val someCodeBlock = """
  foo()
"""

Your Environment

  • Version of ktlint used: 0.44.0
  • Name and version (or code for custom task) of integration used (Gradle plugin, Maven plugin, command line, custom Gradle task): Gradle plugin
  • Link to your project (if it's a public repository): https://github.com/RBusarow/ModuleCheck
@romtsn
Copy link
Collaborator

romtsn commented Feb 20, 2022

I think that's what my concern was exactly about #1262 (comment), but somehow it got ignored. We shouldn't change contents of raw strings if they are not followed by trimIndent/trimMargin

@romtsn romtsn added the bug label Feb 20, 2022
@RBusarow
Copy link
Author

We shouldn't change contents of raw strings if they are not followed by trimIndent/trimMargin

Not even then.

You can safely increase or decrease the indent/margin of existing lines if one of those extensions is used, but adding a new line like this would still result in a change to the string's value.

@paul-dingemans
Copy link
Collaborator

I think that's what my concern was exactly about #1262 (comment), but somehow it got ignored. We shouldn't change contents of raw strings if they are not followed by trimIndent/trimMargin

I can now see what your concerns were. In this case it indeed is wrong to align the closing quotes when not followed by trim indent/trimmargin. As documented on https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.text/trim-indent.html this method (same for trimmargin) do remove the trailing and pending newlines. Of course this does not happen for a raw string without them.

@paul-dingemans
Copy link
Collaborator

@RBusarow Is your problem just an abstraction for a real problem in production code or just a semantic discussion about how you want to write unit tests? Semantically, you are still right, that this is unwanted behavior.

val someCodeBlock = """
  foo()"""

If above would be used in some production code like:

val message = """
  some-message"""

then this message would not not contain a newline at the beginning but also the indentation before the text some-message.

See unit tests below, which clearly identifies the behavior between the approaches:

@Test
fun `Given a multiline raw string without trimIndent contains a new line`() {
    val message = """
        some-message"""

    assertThat(message).isEqualTo("\n            some-message")
}

@Test
fun `Given a multiline raw string with trimIndent does not contain leading or trailing new lines`() {
    val message = """
        some-message
    """.trimIndent()

    assertThat(message).isEqualTo("some-message")
}

So is it really likely that this causes problems in production code?

@RBusarow
Copy link
Author

@paul-dingemans That's just an abstraction. My real-world example of 0.44.0's changes is here: https://github.com/RBusarow/ModuleCheck/pull/393/files.

This project parses Java, Kotlin, Groovy, and XML files, and performs auto-corrections in Groovy and Kotlin Gradle files. It's important that whitespaces are handled correctly in tests, as well as in the changes being made to the Gradle files.

The changes to this rule do not affect production code, but they affect my ability to test the production code.

@gchallen
Copy link

gchallen commented Feb 23, 2022

I'm having the same problem. 3.9.0 broke a bunch of tests that were working previously due to modifying raw string templates. It broke both tests, and the production code itself. I'd vote for reverting this behavior, or at least putting it behind a feature flag to let maintainers catch up to this change.

(NB: I'm using the org.jmailen.kotlinter, and it looks like version 3.9.0 brought in this change.)

@paul-dingemans
Copy link
Collaborator

@RBusarow @gchallen I am gonna fix this in the next release.

Meanwhile you can have a look at the IndentationRuleTest in ktlint. For example in

fun `multiline string with mixed indentation characters, can not be autocorrected`() {
we use special markers when tabs, spaces or the raw string literal quotes are essential in the test. This way you can make those characters very explicit and easily readable.

@jrogers
Copy link

jrogers commented Mar 8, 2022

I saw this error in some code as well after updating. In this scenario the string literals (they were format templates) were defined as const and were relying on the specific whitespace formatting because the trimIndent() call couldn't be added without making them non-const.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment