pinterest / ktlint

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

argument-list-wrapping stops working with too many arguments #2461

Closed mklnwt closed 10 months ago

mklnwt commented 11 months ago

argument-list-wrapping doesn't work for 9 or more arguments.

It's arguable if a function should have that many parameters, but the same is also failing for constructor calls (e.g.. data classes).

Expected Behavior

val code =
    """
    val x = f(1, 2, 3, 4, 5, 6, 7, 8, 9)
    """.trimIndent()
val formattedCode =
    """
    val x = f(
        1,
        2,
        3,
        4,
        5,
        6,
        7,
        8,
        9
    )
    """.trimIndent()
argumentListWrappingRuleAssertThat(code)
    .withEditorConfigOverride(MAX_LINE_LENGTH_PROPERTY to 30)
    .isFormattedAs(formattedCode)

Observed Behavior

The example above fails. Below works though.

val code =
    """
    val x = f(1, 2, 3, 4, 5, 6, 7, 8)
    """.trimIndent()
val formattedCode =
    """
    val x = f(
        1,
        2,
        3,
        4,
        5,
        6,
        7,
        8
    )
    """.trimIndent()
argumentListWrappingRuleAssertThat(code)
    .withEditorConfigOverride(MAX_LINE_LENGTH_PROPERTY to 30)
    .isFormattedAs(formattedCode)

Your Environment

paul-dingemans commented 11 months ago

Yes, I am aware of that (see https://github.com/pinterest/ktlint/blob/master/ktlint-ruleset-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/rules/ArgumentListWrappingRule.kt#L115).

It has been added in this commit by another maintainer (I did not yet joined the project then). Unfortunately, there is no rationale documented for it. Simply removing it, may lead to complaints of other users that rely on it. It has been added in ktlint 0.38.0, August 2020. I am not aware of any other complaints about this. Based on this, I see little reason to change it.

mklnwt commented 11 months ago

This seems rather random.

Would it be possible to add a configuration instead and default it to 8?

paul-dingemans commented 11 months ago

This seems rather random.

Yes it is. But I think I have found the reason why it was added without an issue. Ktlint contains quite a few statements like below:

private val tokenSet =
    TokenSet.create(
        MUL, PLUS, MINUS, DIV, PERC, LT, GT, LTEQ, GTEQ, EQEQEQ, EXCLEQEQEQ, EQEQ,
        EXCLEQ, ANDAND, OROR, ELVIS, EQ, MULTEQ, DIVEQ, PERCEQ, PLUSEQ, MINUSEQ, ARROW,
    )

So most likely while running the rules on the own code base, it turned out that this would to huge code blocks if each element would be wrapped to a separate line.

Would it be possible to add a configuration instead and default it to 8?

Yes, technically not a problem. But as said, this random number has now been here for over 3 years and you're the first who wants it get changed. Each configuration setting can lead to more discussion between team members about what the best value is.

What would your ideal value be, and more importantly why specific that value?

mklnwt commented 11 months ago

Adding the property would be for legacy/backwards compatibility reason, addressing your comment it cannot simply be removed.

My preference would be to remove that 8. If a new configuration gets added for it, I would set it to 0/unset to always wrap when max-line-length is reached..

paul-dingemans commented 11 months ago

My preference would be to remove that 8. If a new configuration gets added for it, I would set it to 0/unset to always wrap when max-line-length is reached..

Lol, this is probably the only valid argument for this configuration setting. And in case of a list of arguments which get to long, the rule can always be suppressed for that statement:

private val tokenSet =
    @Suppress("ktlint:standard:argument-list-wrapping")
    TokenSet.create(
        MUL, PLUS, MINUS, DIV, PERC, LT, GT, LTEQ, GTEQ, EQEQEQ, EXCLEQEQEQ, EQEQ,
        EXCLEQ, ANDAND, OROR, ELVIS, EQ, MULTEQ, DIVEQ, PERCEQ, PLUSEQ, MINUSEQ, ARROW,
    )

Let me consider whether I will set the default for ktlint_official code style to unset. Users of this code style seem to be a bit more lenient for changes like this. For the other code styles the default will be kept at current value 8.

mklnwt commented 11 months ago

The would definitely be enough for me.

I'm only interested in auto-formatting when there are more than 8 arguments.