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

Ktlint reporting unexpected indentation when formatting lambdas with trailing comma #1247

Closed
matejdro opened this issue Oct 8, 2021 · 6 comments · Fixed by #1293
Closed

Comments

@matejdro
Copy link

matejdro commented Oct 8, 2021

Following code:

fun test() {
    lateinit var a: (String, String, String) -> String

    a = {
            s: String,
            s1: String,
            s2: String,
        ->
        TODO()
    }
}

has been formatted with default IntelliJ kotlin code style + trailing comma enabled

Expected Behavior

ktlint should accept the code

Observed Behavior

ktlint fails with

Test.kt:5:1 Unexpected indentation (12) (should be 8) (indent)
Test.kt:6:1 Unexpected indentation (12) (should be 8) (indent)
Test.kt:7:1 Unexpected indentation (12) (should be 8) (indent)

Your Environment

  • Version of ktlint used: 0.42.1
  • Name and version (or code for custom task) of integration used (Gradle plugin, Maven plugin, command line, custom Gradle task): ktlint-gradle 10.2.0
@matejdro
Copy link
Author

matejdro commented Oct 8, 2021

Even worse, due to the #967, there appears to be no workaround for this (other than disabling indent rule globally).

@Kernald
Copy link

Kernald commented Nov 3, 2021

This also seems to not be playing well with #1032 at all.

@paul-dingemans
Copy link
Collaborator

This is really an interesting code sample. The trailing comma after variable s2 makes a huge difference for the formatting.

When code style preference Use trailing comma in IntelliJ is disabled and s2 is not followed by a trailing comma , the code gets formatted as:

fun test() {
    lateinit var a: (String, String, String) -> String

    a = { s: String,
          s1: String,
          s2: String
        ->
        TODO()
    }
}

But in both cases the IntelliJ formatting looks better than the current formatting of Ktlint shown below:

fun test() {
    lateinit var a: (String, String, String) -> String

    a = {
        s: String,
        s1: String,
        s2: String
        ->
        TODO()
    }
}

I think we should use the same formatting style in both cases. I prefer the style which is used when no trailing comma is found, so:

fun test() {
    lateinit var a: (String, String, String) -> String

    a = { s: String, 
          s1: String,
          s2: String
        ->
        TODO()
    }
}

@romtsn @Tapchicoma What do you think?

paul-dingemans pushed a commit to paul-dingemans/ktlint that referenced this issue Nov 28, 2021
@paul-dingemans
Copy link
Collaborator

I prepared a fix but have to come back on my preference above. This looks prettier in this example due to the extremely short variable name a. With longer variable names, it is better to stick to format:

fun test() {
    lateinit var a: (String, String, String) -> String

    a = {
            s: String,
            s1: String,
            s2: String,
        ->
        TODO()
    }
}

@matejdro
Copy link
Author

Can we even decide on the format, though? I assume whatever IntelliJ's formatter spits out, it must also be valid ktlint formatting? Since, from what I can see, there is no way to tell IntelliJ's formatter to format it like that.

@paul-dingemans
Copy link
Collaborator

For me the only requirements are:

  • Output of ktlint should be accepted by IntelliJ formatting rules
  • ktlint should format consistently.

Code formatted by fix #1293 applies to those rules.

shashachu added a commit that referenced this issue Dec 20, 2021
Closes #1247

Co-authored-by: Paul Dingemans <pdingemans@bol.com>
Co-authored-by: Sha Sha Chu <shasha@pinterest.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants