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

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

Closed
mklnwt opened this issue Dec 23, 2023 · 6 comments · Fixed by #2481
Closed

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

mklnwt opened this issue Dec 23, 2023 · 6 comments · Fixed by #2481
Milestone

Comments

@mklnwt
Copy link

mklnwt commented Dec 23, 2023

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

  • Version of ktlint used: 1.1.0 (tested on master as well)
@paul-dingemans
Copy link
Collaborator

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
Copy link
Author

mklnwt commented Dec 23, 2023

This seems rather random.

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

@paul-dingemans
Copy link
Collaborator

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
Copy link
Author

mklnwt commented Dec 24, 2023

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
Copy link
Collaborator

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.

@paul-dingemans paul-dingemans added this to the 1.2 milestone Dec 24, 2023
@mklnwt
Copy link
Author

mklnwt commented Dec 24, 2023

The would definitely be enough for me.

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

paul-dingemans added a commit that referenced this issue Jan 1, 2024
… treshold of parameters

Breaking change: For code style `ktlint_official` the default value for this setting has default `unset` which is different from previous default value `8`. Not wrapping parameters should be a conscious decision of the developer. This can either be done on a case by case basis by suppressing the rule, or by configuring the parameter.

Closes #2461
paul-dingemans added a commit that referenced this issue Jan 23, 2024
… treshold of parameters (#2481)

* Add configuration setting for ignoring `argument-list-wrapping` above treshold of parameters

Breaking change: For code style `ktlint_official` the default value for this setting has default `unset` which is different from previous default value `8`. Not wrapping parameters should be a conscious decision of the developer. This can either be done on a case by case basis by suppressing the rule, or by configuring the parameter.

Closes #2461
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants