pinterest / ktlint

An anti-bikeshedding Kotlin linter with built-in formatter
https://pinterest.github.io/ktlint/
MIT License
6.06k stars 504 forks source link

Indentation of inheritance with parameters #2661

Closed mklnwt closed 1 month ago

mklnwt commented 1 month ago

Indentation for inheritance implementation does not look correct.

For example

class Bar(
    baz: String,
) : Foo(
        baz = baz,
    ) {
    fun qux() { /* ... */ }
}

Expected Behavior

@Test
fun `Given a class with inheritance`() {
    val code =
        """
        class Bar(
            baz: String,
        ) : Foo(
            baz = baz,
        )
        """.trimIndent()
    indentationRuleAssertThat(code).hasNoLintViolations()
}

Current Behavior

@Test
fun `Given a class with inheritance`() {
    val code =
        """
        class Bar(
            baz: String,
        ) : Foo(
                baz = baz,
            )
        """.trimIndent()
    indentationRuleAssertThat(code).hasNoLintViolations()
}

Additional information

paul-dingemans commented 1 month ago

Code style ktlint_official takes a different perspective than you might be used. From a consistency viewpoint the new format allows better formatting in case the class extends a super class having arguments as well as some additional interfaces. Code below is consistently formatted:

class Foo(
    foo1: Foo1,
    foo2: Foo2,
) : Foobar1(
        "foobar1",
        "foobar2",
    ),
    Foobar2,
    Foobar3 {
    fun foo() = "foo"
}

The super class Foobar1 and interfaces Foobar2 and Foobar3 are all aligned without the closing parenthesis and comma of Foobar1 disturbing the formatting.

With the other code styles, the old way of formatting is still available:

class Foo(
    foo1: Foo1,
    foo2: Foo2
) : Foobar1(
    "foobar1",
    "foobar2"
),
    FooBar2,
    FooBar3 {
    fun foo() = "foo"
}

Code style ktlint_official will always choose the most consistent formatting possible. If might take some time to get used to.

mklnwt commented 1 month ago

Considering that function calls look like this:

foos.map {
    it + 1
}.bar()
    .baz()

or

foo(
    bar = "bar",
).bar()
    .baz()

I would say that the inheritance example is not fully consistent.

It becomes even weirder. This is the formatted behavior when breaking the first inheritance:

class Foo(
    foo1: Int,
    foo2: Int,
) :
    Foobar1(
            a = 1,
            b = 2,
        ),
        Foobar2,
        Foobar3 {
    fun foo() = "foo"
}

Otherwise is it possible to use ktlint_official, but switch this specific rule part to the old style?

paul-dingemans commented 1 month ago

I think we have had discussions like this before. You have to disclose your .editorconfig settings. Above is most likely the result of disabling certains rules or use non-default settings.

With .editorconfig below, the code above is not accepted, and it is formatted consistently.

root = true

[*.{kt,kts}]
ktlint_code_style = ktlint_official
ktlint_standard = enabled
ktlint_experimental = enabled

Otherwise is it possible to use ktlint_official, but switch this specific rule part to the old style?

No that is not supported, and will not be supported.

mklnwt commented 1 month ago

@paul-dingemans I have mostly worked with the unit test provided above.

I experimented a bit locally and it seems that a missing experimental rule is the reason the join to the previous line is not enforced.

I still believe that the indentation rule by itself (deactivated Expected single space before the first super type (standard:class-signature)) should not produce the example above.

This config for 1.2.1:

root = true

[*.{kt,kts}]
ktlint_code_style = ktlint_official
ktlint_standard = enabled
paul-dingemans commented 1 month ago

I experimented a bit locally and it seems that a missing experimental rule is the reason the join to the previous line is not enforced.

Ah good remark. Indeed the chain-method-continuation rule is crucial for the "correct" formatting. This can be easily found, using Ktlint CLI:

$ ktlint-1.2.1 --stdin
16:19:58.278 [main] INFO com.pinterest.ktlint.cli.internal.KtlintCommandLine -- Enable default patterns [**/*.kt, **/*.kts]
val foos =
    foos.map {
        it + 1
    }.bar()
    .baz()
<stdin>:2:9: Expected newline before '.' (standard:chain-method-continuation)
<stdin>:5:1: Unexpected indentation (4) (should be 8) (standard:indent)

Summary error count (descending) by rule:
  standard:chain-method-continuation: 1
  standard:indent: 1

The chain-method-continuation becomes a non-experimental rule in the upcoming release. The code above will then be formatted as:

val foos =
    foos
        .map {
            it + 1
        }.bar()
        .baz()

Your second example is:

foo(
    bar = "bar",
).bar()
    .baz()

I have no idea how this should be written more nicely, given that the arguments of foo are to be wrapped to separate lines.

For shorter chains, the following is accepted by ktlint and might be a workaround:

val foos =
    foo(
        bar = "bar",
    ).bar().baz()

Another, but worse workaround, is to write it like:

val foos =
    foo(
        bar = "bar",
    ).apply {
        barbarbarbarbarbarbarbarbarbarbarbar()
            .bazbazbazbazbazbazbazbazbazbazbaz()
    }

For now this example can not be marked as a 'bug', until a solution can be defined what formatting would work.

Your last example is caused by missing the class-signature rule which also becomes non-experimental in the upcoming release:

$ ktlint-1.2.1 --stdin
16:34:13.568 [main] INFO com.pinterest.ktlint.cli.internal.KtlintCommandLine -- Enable default patterns [**/*.kt, **/*.kts]
class Foo(
    foo1: Int,
    foo2: Int,
) :
    Foobar1(
        a = 1,
        b = 2,
    ),
    Foobar2,
    Foobar3 {
    fun foo() = "foo"
}
<stdin>:5:5: Expected single space before the first super type (standard:class-signature)
<stdin>:6:1: Unexpected indentation (8) (should be 12) (standard:indent)
<stdin>:7:1: Unexpected indentation (8) (should be 12) (standard:indent)
<stdin>:8:1: Unexpected indentation (4) (should be 8) (standard:indent)
<stdin>:9:1: Unexpected indentation (4) (should be 8) (standard:indent)
<stdin>:10:1: Unexpected indentation (4) (should be 8) (standard:indent)

Summary error count (descending) by rule:
  standard:indent: 5
  standard:class-signature: 1

You're right that the behavior of the indent rule without the class-signature can be seen as a bug. But with class-signature becoming a non-experimental rule this will be prevented. Only if the class-signature is disabled, this problem will re-appear again but that is than a side effect that is to be accepted.

mklnwt commented 1 month ago

I think the last comment was in the wrong ticket or rather a mix between tickets.

In general I reckon we can close these tickets for now.

Sidenote and personal opinion: Breaking after the assignment AND at the first method call seems very inelegant and wasting a lot of space (both vertically and horizontally).

val foos =
    foos
        .foo()
        .bar()
        .baz()
        .qux()

seems a lot less readable than

val foos = foos.foo()
    .bar()
    .baz()
    .qux()
paul-dingemans commented 1 month ago

Sidenote and personal opinion: Breaking after the assignment AND at the first method call seems very inelegant and wasting a lot of space (both vertically and horizontally).

With short identifiers it seems indeed a waste of space. In real life, identifiers can also become (very) long.

val foos =
    foosfoosfoosfoosfoosfoosfoosfoosfoosfoosfoosfoos
        .foofoofoo()
        .bar()
        .baz()
        .qux()

which makes foofoofoo harder to discover

val foos = foosfoosfoosfoosfoosfoosfoosfoosfoosfoosfoosfoos.foofoofoo()
    .bar()
    .baz()
    .qux()

As with a lot of decisions regarding formatting it is subjective what reads better. I always try to look at extreme examples when to decide what format to use.