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

Add call argument list wrapping rule #318

Closed

Conversation

marvin-roesch
Copy link

@marvin-roesch marvin-roesch commented Nov 26, 2018

Using ktlint in a recent project, I noticed that there only is validation for parameter formatting at declaration sites (both for functions and classes). As a consequence, ktlint will not report code such as the following:

myfun(paramA,
    paramB, paramC,
       paramD,
    paramE)

Running the IntelliJ IDEA formatter with the ktlint-provided settings on this code yields this result:

myfun(
    paramA,
    paramB, paramC,
    paramC,
    paramD
)

In order to keep ktlint in line with this behaviour, an argument list wrapping rule is provided in the standard ruleset. This also addresses #222 and the case brought up there (multiple arguments on the same line should stay there unless they exceed the maximum line length).

Note: I based the code heavily off of the parameter list wrapping rule. In order to reduce some redundancy, I moved the shared functions of both rules to the package.kt file.

@shyiko
Copy link
Collaborator

shyiko commented Dec 4, 2018

Thank you, Marvin! ❤️

@shyiko
Copy link
Collaborator

shyiko commented Dec 4, 2018

woops, I was about to merge the PR but noticed the build has failed. Any chance you can take a look?

@marvin-roesch
Copy link
Author

Looks like it's my new rule triggering for the ktlint code. I'll go through the code base manually to fix these issues (some of them seem to be not suited for automatic formatting) when I find the time!

@shashachu
Copy link
Contributor

@PaleoCrafter sorry for letting this sit so long. When I run the IDEA formatter on a similar function call, it doesn't put the first parameter on a newline. Is that some option somewhere? I just want to make sure we're consistent.

@yshrsmz
Copy link

yshrsmz commented Jul 3, 2019

@shashachu

Hi, I'm not the author but there is a New line after '(' option in Function call arguments section.

I've been searching for an equivalent of this option in ktlint and found this PR. It would be great if this gets merged.

screenshot2019-07-03 10 46 12

@shashachu
Copy link
Contributor

@shashachu

Hi, I'm not the author but there is a New line after '(' option in Function call arguments section.

I've been searching for an equivalent of this option in ktlint and found this PR. It would be great if this gets merged.

screenshot2019-07-03 10 46 12

@yshrsmz have you tried running ktlint with the --experimental flag? That turns on the IndentationRule, which is more aggressive about adding in newlines after function calls.

@marvin-roesch
Copy link
Author

@shashachu, sorry for letting this sit unaddressed for over a month in turn! I've based the rule's behaviour on the Kotlin style guide and IDEA's behaviour when set to format using the guide. See the section on method calls for reference.

@shashachu
Copy link
Contributor

@PaleoCrafter actually this rule's functionality has already been folded into the experimental IndentRule, which will be promoted into the StandardRuleset soon.. I just verified that running ktlint --experimental -F on your code snippet does output the properly-formatted function call.

@marvin-roesch
Copy link
Author

@shashachu: Ah, I guess the IndentationRule does subsume my rule completely. It does behave a little differently, in that it always demands a newline before/after parentheses in a call containing a multiline lambda. This seems to diverge from IntelliJ's formatting behaviour.

A simple test case is as follows:

println(listOf(1, 2).joinToString {
    it.toString()
})

This does not get changed by IDEA nor my rule, but the indentation rule puts the argument on a new line. The Jetbrains conventions document isn't explicit enough to decide on a "correct" behaviour here.

Feel free to close my PR, but maybe you can extract something from my rule to deal with the above issue.

@marvin-roesch
Copy link
Author

Oh, something I guess my rule has over the indentation one: It supports chopping down when the maximum line length gets exceeded. If I can factor out that behavior only, does this PR still have a chance?

@shashachu
Copy link
Contributor

Oh, something I guess my rule has over the indentation one: It supports chopping down when the maximum line length gets exceeded. If I can factor out that behavior only, does this PR still have a chance?

It'd be cool to build max line length-awareness into the IndentationRule eventually. Otherwise, I don't think there's anything else in this PR that needs to be in a separate rule. I'm going to go ahead and close it.

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