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

Fixes #620 by adding arguments to ParameterListWrappingRule. #786

Merged
merged 11 commits into from
Aug 5, 2020

Conversation

AdamMc331
Copy link
Contributor

@AdamMc331 AdamMc331 commented Jun 15, 2020

Reference ticket #620 for details. This now considers anything that is VALUE_ARGUMENT in addition to VALUE_PARAMETER for the ParameterListWrappingRule. This means function calls will get the same treatment as class definitions for their arguments being on separate lines (if necessary).

Example

Before, ktlint considered this valid:

fun callTest() {
    test(
        "One",
        "Two", "Three", "Four"
    )
}

This is not clear, though, as multiple arguments on one line is confusing, if they're not all on one line. Now it will auto format them to be on separate lines:

fun callTest() {
    test(
        "One",
        "Two", 
        "Three", 
        "Four"
    )
}

One Concern

All of the tests pass, but the annotation one is a little confusing as for me it appears it wanted to format the annotation like this:

class A {
    fun f(
        // ...
        @Annotation(
            [
            "v1",
            "v2"
        ]
        )
        // ...
}

Some back and forth with @romtsn on the ticket right now though to see if that's an issue before we merge this.

@AdamMc331 AdamMc331 marked this pull request as draft June 15, 2020 17:11
@romtsn
Copy link
Collaborator

romtsn commented Jul 13, 2020

hey @AdamMc331, are you planning to finish this up or should we take over?

@AdamMc331
Copy link
Contributor Author

Hey @romtsn Sorry for falling off on this again. I still have an issue formatting this if there's a argument list inside an annotation. I will clean up my git history and write a test to demonstrate the bug, but I'd definitely appreciate some help looking into it I'm at a loss.

@romtsn
Copy link
Collaborator

romtsn commented Jul 28, 2020

I made it work, but there's another corner case, which we probably should consider and I'm looking into it now:

class NoVarRuleTest : Spek({
    // this should not fail
})

@AdamMc331
Copy link
Contributor Author

👀 Exciting! Thanks for your help pushing this across the finish line.

@AdamMc331 AdamMc331 marked this pull request as ready for review July 30, 2020 21:48
@romtsn
Copy link
Collaborator

romtsn commented Aug 2, 2020

I am a bit hesitant to release this in the standard ruleset, as although the change is not significant code-wise, it kind of has a big impact even on ktlint's codebase.. maybe we should move it to the experimental ruleset for the next release? Thoughts @Tapchicoma @shashachu @AdamMc331? Otherwise, it's ready to be merged.

@AdamMc331
Copy link
Contributor Author

I'm open to the experimental ruleset but that could also be the imposter syndrome talking. 😂 This is my first contribution to a ktlint ruleset and so I'm definitely not as familiar as the others tagged but I will support their opinion either way.

If we go with the experimental ruleset, what is the process like for getting it moved out of experimental in the future?

@shashachu
Copy link
Contributor

Although it may aggravate some people, I don't think it's a big deal to leave this in the standard ruleset, as its functionality is more consistent with the IndentRule, which is quite aggressive about adding newlines.

@shashachu
Copy link
Contributor

@AdamMc331 does this PR also address #680?

@romtsn
Copy link
Collaborator

romtsn commented Aug 4, 2020

@shashachu nope, that problem is specific to indentation rule

@romtsn romtsn merged commit a94b43f into pinterest:master Aug 5, 2020
@romtsn
Copy link
Collaborator

romtsn commented Aug 5, 2020

Thanks Adam!

@AdamMc331
Copy link
Contributor Author

Thanks everyone! :)

@AdamMc331 AdamMc331 deleted the KTL-620/argument_wrapping branch August 5, 2020 13:17
@adamu
Copy link

adamu commented Aug 26, 2020

I’m seeing lots of results for ParameterListWrapping after upgrading to detekt 1.12.0 that look either questionable or wrong. Could it be related to this PR? A couple of examples:

Questionable. Fails with Argument should be on a separate line (unless all arguments can fit a single line) and Missing newline before ")" .

foo(bar.apply {
    // stuff
})

Wrong? Fails function(arg1, arg2, arg3) with Argument should be on a separate line (unless all arguments can fit a single line).

json(
    """
    {
        "array": [
            ${function(arg1, arg2, arg3)}
        ]
    }
    """.trimIndent()
)

@romtsn
Copy link
Collaborator

romtsn commented Aug 26, 2020

@adamu The latter one looks pretty much like #859, the former one though should not be reported (at least the arguments part). Could you create a separate issue for the former one? Thanks

@adamu
Copy link

adamu commented Aug 27, 2020

@adamu The latter one looks pretty much like #859, the former one though should not be reported (at least the arguments part). Could you create a separate issue for the former one? Thanks

Done! #861

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 this pull request may close these issues.

4 participants