pinterest / ktlint

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

Insert blank lines for `when` statement branches #2533

Closed Goooler closed 7 months ago

Goooler commented 8 months ago

Expected Behavior

For intellij_idea style:

https://kotlinlang.org/docs/coding-conventions.html#control-flow-statements

In a when statement, if a branch is more than a single line, consider separating it from adjacent case blocks with a blank line:

private fun parsePropertyValue(propName: String, token: Token) {
    when (token) {
        is Token.ValueToken ->
            callback.visitValue(propName, token.value)

        Token.LBRACE -> { // ...
        }
    }
}

Current Behavior

No blank lines inserted for the example above.

Additional information

paul-dingemans commented 8 months ago

In a when statement, if a branch is more than a single line, consider separating it from adjacent case blocks with a blank line:

The crucial part in this description is the word 'consideration', which to me sounds like something that could be considered from case to case. On the other hand, the default IntelliJ IDEA formatter seems to apply this always which could be an argument to implement this for intellij_idea code style. I am not really convinced that it is a good idea to implement in the same way as in the default IntelliJ IDEA formatter.

Given code below:

private fun parsePropertyValue(propName: String, token: Token) {
    when (token) {
        Token.RBRACE -> Unit
        is Token.ValueToken ->
            callback.visitValue(propName, token.value)
        Token.LBRACE -> { // ...
        }
    }
}

which is formatted by default Intellij IDEA formatter to:

private fun parsePropertyValue(propName: String, token: Token) {
    when (token) {
        Token.RBRACE -> Unit
        is Token.ValueToken ->
            callback.visitValue(propName, token.value)

        Token.LBRACE -> { // ...
        }
    }
}

In find it less readable, or more messy, as the multiline when condition is now clearly separated while the other when conditions are not separated by a newline. Of course, a manual newline can be added as shown below, which is respected by the default formatter:

private fun parsePropertyValue(propName: String, token: Token) {
    when (token) {
        Token.RBRACE -> Unit

        is Token.ValueToken ->
            callback.visitValue(propName, token.value)

        Token.LBRACE -> { // ...
        }
    }
}

But still it can be cleaner / more consistent by using the same style in each when condition:

private fun parsePropertyValue(propName: String, token: Token) {
    when (token) {
        Token.RBRACE ->
            Unit

        is Token.ValueToken ->
            callback.visitValue(propName, token.value)

        Token.LBRACE -> {
            // ...
        }
    }
}

or

private fun parsePropertyValue(propName: String, token: Token) {
    when (token) {
        Token.RBRACE -> {
            Unit
        }

        is Token.ValueToken -> {
            callback.visitValue(propName, token.value)
        }

        Token.LBRACE -> {
            // ...
        }
    }
}
Goooler commented 8 months ago

Yeah, as you described, the code below

fun foo(bar: Any) {
  when (bar) {
    is String -> {
      println("input is a string")
    }

    is Int -> println("input is an int")
    else -> Unit
  }
}

will be formatted to

fun foo(bar: Any) {
  when (bar) {
    is String -> {
      println("input is a string")
    }

    is Int -> println("input is an int")
    else -> Unit
  }
}

we can also add an extra blank line between the second branch and the third branch, like

fun foo(bar: Any) {
  when (bar) {
    is String -> {
      println("input is a string")
    }

    is Int -> println("input is an int")

    else -> Unit
  }
}

which will also be respected by IntelliJ formatter.

There would be a new rule to control this, if a branch is more than a single line in a when, each blank line will be inserted between branches.

paul-dingemans commented 8 months ago

And how do you feel about consistent using braces if at least one branch has braces? Something similar is done for if-else.

Goooler commented 8 months ago

I disagree with that. For example:

fun foo(input: Any) {
  when (input) {
    is String -> foo(input)

    is Int -> {
      println("input is an int")
    }

    else -> Unit
  }
}

the first branch is just invoking foo, which with clear logic, no need to warp it with braces for increasing etra 2 lines.

paul-dingemans commented 7 months ago

I have asked for feedback via a post on LinkedIn

Original code:

// Simple when-statement
fun foo(bar: Bar) =
    when (bar) {
        ABC_ABC_ABC_ABC -> "Lorem ipsum 1"
        DE -> 
            "Lorem ipsum 2"
        KLMNOPQ -> "Lorem ipsum 4"
        else -> null
    }

Proposal 1 (default IDEA formatting):

// Proposal 1
fun foo(bar: Bar) =
    when (bar) {
        ABC_ABC_ABC_ABC -> "Lorem ipsum 1"
        DE ->
            "Lorem ipsum 2"

        FGHIJ -> {
            """
            Lorem ipsum 3
            """.trimIndent()
        }

        KLMNOPQ -> "Lorem ipsum 4"
        else -> null
    }

Proposal 2 (blank lines before each when-condition):

// Proposal 2
fun foo(bar: Bar) =
    when (bar) {
        ABC_ABC_ABC_ABC -> "Lorem ipsum 1"

        DE ->
            "Lorem ipsum 2"

        FGHIJ -> {
            """
            Lorem ipsum 3
            """.trimIndent()
        }

        KLMNOPQ -> "Lorem ipsum 4"

        else -> null
    }

Proposal 3 (blank lines before each when-condition + wrapping inside braces):

// Proposal 3
fun foo(bar: Bar) =
    when (bar) {
        ABC_ABC_ABC_ABC -> {
            "Lorem ipsum 1"
        }

        DE -> {
            "Lorem ipsum 2"
        }

        FGHIJ -> {
            """
            Lorem ipsum 3
            """.trimIndent()
        }

        KLMNOPQ -> {
            "Lorem ipsum 4"
        }

        else -> {
            null
        }
    }

The results sofar are:

Interesting other remarks:

paul-dingemans commented 7 months ago

The default IDEA formatter uses setting ij_kotlin_line_break_after_multiline_when_entry to determine whether a blank line is to be inserted after a multiline when-condition. Ktlint can use this setting to force a blank line after each when-condition (regardless whether it is a single or multiline condition) in case the when-statement contains at least one multiline-condition. When the setting is disabled, the blank lines between the when-conditions are removed.

Another rule can be added to enforce the use of braces in case the condition is a multiline statement including when-condition like below:

fun foo(bar: Bar) =
    when (bar) {
        // ...

        TWO ->
            "Lorem ipsum 2"

        // ...
    }
Goooler commented 7 months ago

From my observation, blank lines will be inserted after each right curly brace, proposal 1 and 2 are both respected by IDEA, I'd suggest we can regard them as correct.

paul-dingemans commented 7 months ago

From my observation, blank lines will be inserted after each right curly brace, proposal 1 and 2 are both respected by IDEA, I'd suggest we can regard them as correct.

Yes, Intellij IDEA accept both solutions. Ktlint favors consistency. Either you have a blank line between when entries, or you have none.

eygraber commented 7 months ago

I can live with what the new rule outputs, but it would be really nice if I could have something like this:

"a" -> {
  Unit
  Unit
}

"b" -> {
  Unit
  Unit
}

"c" -> Unit
"d" -> Unit
"e" -> Unit

Aside from that, it would've been nice to find out about this before it was released. Kind of odd that this was only asked in a LinkedIn post, when there's a ktlint channel in the Kotlin Slack, and channels that are meant for x-posting these types of things that have more developers in it than that LinkedIn group.

paul-dingemans commented 7 months ago

I can live with what the new rule outputs, but it would be really nice if I could have something like this:

In my opionion this is not consistent. Therefore it will not be added. Also from my investigation on LinkedIn, it was not mentioned as a favored option.

Aside from that, it would've been nice to find out about this before it was released. Kind of odd that this was only asked in a LinkedIn post, when there's a ktlint channel in the Kotlin Slack, and channels that are meant for x-posting these types of things that have more developers in it than that LinkedIn group.

Previously the ktlint slack channel proofed not to be the medium that give me the feedback when I needed. Users of ktlint seem to reach out to the channel when they need, but hardly seem to be engaged in discussion. I will use whatever medium I see fit to collect feedback.