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

Indentation change since 0.38.1 #1217

Closed
dalewking opened this issue Aug 18, 2021 · 15 comments · Fixed by #1872
Closed

Indentation change since 0.38.1 #1217

dalewking opened this issue Aug 18, 2021 · 15 comments · Fixed by #1872
Labels
conflict-with-default-intellij-formatting Code produced by KtLint is not accepted by the IntelliJ default formatter indentation rule ktlint-official-codestyle
Milestone

Comments

@dalewking
Copy link

Expected Behavior

Upgrading from 0.38.1 to latest I see a change in indentation with parameter names in a call.

For example this was allowed in 0.38.1:

someFunction(
    parameterName =
        someValue
            .someProperty
            .someCall()
)

Observed Behavior

With 0.41.1 it now demands that it be indented like this:

someFunction(
    parameterName =
    someValue
        .someProperty
        .someCall()
)

Which is not very readable.

I can make it go away by moving someValue to same line as parameter name:

someFunction(
    parameterName = someValue
        .someProperty
        .someCall()
)

But sometimes there is a longer expression than this example and it is more readable to move it to its own line.

@vojkny
Copy link

vojkny commented Sep 20, 2021

can sb pls look at this?

@paul-dingemans
Copy link
Collaborator

Problem is not reproducible in ktlint 0.43.x

@vojkny
Copy link

vojkny commented Dec 11, 2021

It still hapens for me once I use the --experimental parameter on 0.43.2.

@paul-dingemans
Copy link
Collaborator

It still hapens for me once I use the --experimental parameter on 0.43.2.

I double checked it once more on my site, but it works fine. Please provide me details about following:

  • java version
  • editorconfig
  • full code sample in which you experiment the problem when it is not identical to the sample given in this issue
  • full output when running with flags --format --verbose --relative --experimental --debug

@vojkny
Copy link

vojkny commented Dec 12, 2021

trying this:

$ java --version
openjdk 17.0.1 2021-10-19
OpenJDK Runtime Environment Temurin-17.0.1+12 (build 17.0.1+12)
OpenJDK 64-Bit Server VM Temurin-17.0.1+12 (build 17.0.1+12, mixed mode, sharing)
  1. no editorconfig, only single example file
private fun transactionOf() = PurchaseTransaction().also {
    it.timestamp = timestamp
    it.timestampDesk = timestamp
    it.sale = testSale
    it.count = count
    it.amountCents = amountCents
    it.reserved = reserved
    it.dealSplit = mutableListOf(
        PurchaseTransactionDeal().also {
            it.sale = testSale
            it.deal = testDeal
            it.count = count
            it.amountCents = amountCents
        }
    )
}
retina:~/Workspace/ktlint-test$ cat PurchaseInvoiceConsumerTest.kt
private fun transactionOf() = PurchaseTransaction().also {
    it.timestamp = timestamp
    it.timestampDesk = timestamp
    it.sale = testSale
    it.count = count
    it.amountCents = amountCents
    it.reserved = reserved
    it.dealSplit = mutableListOf(
        PurchaseTransactionDeal().also {
            it.sale = testSale
            it.deal = testDeal
            it.count = count
            it.amountCents = amountCents
        }
    )
}
retina:~/Workspace/ktlint-test$ ktlint
retina:~/Workspace/ktlint-test$ ktlint --experimental --format --verbose --relative --debug
[DEBUG] Discovered ruleset with "standard" id.
[DEBUG] Discovered ruleset with "experimental" id.
[DEBUG] Discovered reporter with "baseline" id.
[DEBUG] Discovered reporter with "checkstyle" id.
[DEBUG] Discovered reporter with "json" id.
[DEBUG] Discovered reporter with "html" id.
[DEBUG] Discovered reporter with "plain" id.
[DEBUG] Discovered reporter with "sarif" id.
[DEBUG] Initializing "plain" reporter with {verbose=true, color=false, color_name=DARK_GRAY}
[DEBUG] Checking PurchaseInvoiceConsumerTest.kt
Resolving .editorconfig files for /Users/knyttlwork/Workspace/ktlint-test/PurchaseInvoiceConsumerTest.kt file path
Loaded .editorconfig: []
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by org.jetbrains.kotlin.com.intellij.util.ReflectionUtil (file:/usr/local/Cellar/ktlint/0.43.2/libexec/ktlint) to field java.lang.Throwable.backtrace
WARNING: Please consider reporting this to the maintainers of org.jetbrains.kotlin.com.intellij.util.ReflectionUtil
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
PurchaseInvoiceConsumerTest.kt:9:9: Unexpected indentation (expected 4, actual 8) (experimental:argument-list-wrapping)
PurchaseInvoiceConsumerTest.kt:15:5: Unexpected indentation (expected 0, actual 4) (experimental:argument-list-wrapping)
[DEBUG] 1331ms / 1 file(s) / 2 error(s)

@vojkny
Copy link

vojkny commented Dec 12, 2021

Another failing example is:

val foo = listOf(
    Status.OK example MessageThreadResponse(
        ListResponseMeta(0, true),
        listOf(messageThreadDtoExample),
        MessageThreadResponseIncluded(listOf(), listOf(), listOf()),
    )
)

works correctly without --experimental flag.

@paul-dingemans
Copy link
Collaborator

@knyttl Your examples do not seem to be related to the original problem of @dalewking. Your problem is already addressed in #1284.

@paul-dingemans
Copy link
Collaborator

@dalewking This problem is indeed not solved. Due to the partial code sample, I misinterpreted it before. A complete code sample, reproducing the problem is:

fun someFunction(parameterName: String) =
    parameterName.toUpperCase()

@Test
fun name() {
    val actual = someFunction(
        parameterName =
        "The quick brown fox "
            .plus("jumps ")
            .plus("over the lazy dog")
    )
    assertThat(actual).isEqualTo("THE QUICK BROWN FOX JUMPS OVER THE LAZY DOG")
}

@paul-dingemans
Copy link
Collaborator

I did some further investigation. The current behavior has been built-int on purpose as a result of #964. I have verified in IntelliJ Ultimate and the default formatting for the example above is:

fun someFunction(parameterName: String) =
    parameterName.toUpperCase()

@Test
fun name() {
    val actual = someFunction(
        parameterName =
        "The quick brown fox "
            .plus("jumps ")
            .plus("over the lazy dog")
    )
    assertThat(actual).isEqualTo("THE QUICK BROWN FOX JUMPS OVER THE LAZY DOG")
}

Personally, I would prefer to change the formatting as has been requested. However, it conflicts with the (default) IntelliJ / Android Studio formatting. One of our code policies, is that code that is formatted by ktlint should be accepted by the default IntelliJ formatting without any conflicts.
@romtsn Please confirm, whether you agree on this. If so, this issue should be closed.

@DenWav
Copy link

DenWav commented Dec 18, 2021

The fact that IntelliJ / Android Studio format the code that way shouldn't matter, since it is incorrect according to Jetbrain's own style guide.

Unfortunately they don't have a rule specifically about named parameters, but we can infer from several very similar rules:

Considering the purpose of the style guide is for consistency to begin with, this formatting quirk in IntelliJ is a bug, not a feature to emulate.

@matejdro
Copy link

While I agree with you that ideally IntelliJ should fix its formatter, I don't think it is realistic to ignore IntelliJ's integrated formatter. It would be bad for developer's user experience to turn off integrated IDE formatter and only rely on external ktlint formatter.

@vojkny
Copy link

vojkny commented Dec 20, 2021

However, it conflicts with the (default) IntelliJ / Android Studio formatting.

Could we maybe have some flag for this, which could be disabled? I personally prefer to have a nice code than just follow a weird IDE behaviour.

@paul-dingemans
Copy link
Collaborator

While I agree with you that ideally IntelliJ should fix its formatter, I don't think it is realistic to ignore IntelliJ's integrated formatter. It would be bad for developer's user experience to turn off integrated IDE formatter and only rely on external ktlint formatter.

This issue first needs to be resolved in the IntelliJ formatter. Please file an issue at their tracker first.

Could we maybe have some flag for this, which could be disabled? I personally prefer to have a nice code than just follow a weird IDE behaviour.

Ktlint should not offer such a flag for a minor issue like this.
Most important reason is that it would still conflict with IntelliJ formatting. If IntelliJ would offer such a flag, we could make ktlint compatible with it.
Secondly because the increase of code complexity as well as explaining the configuration settings for such flags becomes bigger and bigger with each flag being added.

@DenWav
Copy link

DenWav commented Dec 25, 2021

This seems backwards to what ktlint should be doing. Adhering to random and unexplained behavior in IntelliJ to the letter instead of examining the situation and choosing the better option ('better' being what everyone here is agreeing with) is not an admirable quality for a formatter like this.

I use ktlint to remove the ambiguity of IDE formatting, not the other way around.

@paul-dingemans paul-dingemans added the conflict-with-default-intellij-formatting Code produced by KtLint is not accepted by the IntelliJ default formatter label Jan 26, 2022
@vojkny
Copy link

vojkny commented Mar 25, 2022

I created an issue for IntelliJ which relates to this: https://youtrack.jetbrains.com/issue/IDEA-291053

@paul-dingemans paul-dingemans added this to the 0.49.0 milestone Mar 16, 2023
paul-dingemans added a commit that referenced this issue Mar 19, 2023
* Allow value arguments with a multiline expression to be indented on a separate line

Closes #1217

* Add rule `multiline-expression-wrapping` which forces multiline expressions to start on a separate line

* Fix lint violations caused by `multiline-expression-wrapping`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conflict-with-default-intellij-formatting Code produced by KtLint is not accepted by the IntelliJ default formatter indentation rule ktlint-official-codestyle
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants