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

Indention Rule: Parameters should be aligned vertically #118

Conversation

MyDogTom
Copy link
Contributor

@MyDogTom MyDogTom commented Dec 4, 2017

Enforce that parameters should be aligned. This is how default IntelliJ
formatter works. This approach is listed on:
https://ktlint.github.io/#rule-indentation
https://kotlinlang.org/docs/reference/coding-conventions.html#class-header-formatting
https://android.github.io/kotlin-guides/style.html#functions

This allows to prevent stuff like that

class ClassA(paramA: String
    paramB: String,
        paramC: String)

@shyiko Sorry, initially I was fully confident that "align parameters" is how rule is designed (https://ktlint.github.io/#rule-indentation). But just before creating PR, I've noticed that error message actually allow not aligned parameters.

Unexpected indentation (${line.length}) (parameters should be either vertically aligned or indented by the multiple of $indent)

Feel free to close this PR, if you actually want to preserve that option.

@MyDogTom
Copy link
Contributor Author

@shyiko This change also allows to implement auto correction. Because expected indent is completely defined. See #126
I'm looking forward to know your thoughts about this change.

@shyiko
Copy link
Collaborator

shyiko commented Dec 12, 2017

Unless I'm reading it wrong,

class ClassA(paramA: String, paramB: String, 
             paramC: String)
class ClassB(paramA: String, paramB: String, paramC: String)

should be formatted as

class ClassA(
    paramA: String, 
    paramB: String, 
    paramC: String
)
class ClassB(paramA: String, paramB: String, paramC: String)

(per "Classes with longer headers should be formatted so that each primary constructor argument is in a separate line with indentation" and "When a function signature does not fit on a single line, break each parameter declaration onto its own line").

Would you be willing to update PR to reflect this? (having this + #126 would be awesome)

# Conflicts:
#	ktlint/src/main/kotlin/com/github/shyiko/ktlint/Main.kt
Parameters should be placed either on one line, either each parameter on new line

Extracted indention size definition in separate class. This allows to avoid duplication between rules
@vanniktech
Copy link
Contributor

Should this really be applied for methods too? Seems like it's a bit of overkill.

@MyDogTom
Copy link
Contributor Author

@shyiko it's even more strict than what I wanted to do. But I'm fine with that :)

I'd like to completely clarify what is done. There are two rules. IndentationRule and ParametersOnSeparateLinesRule. They have different responsibilities.

IndentaionRule only cares about indentation.

In case of parameters, it verifies that, if parameter already has indentation (eg. placed on new line), then parameter should be aligned with the first parameter (regardless where it's placed).

Valid examples

class ClassA(paramA: String, paramB: String, 
             paramC: String)

class ClassA(
    paramA: String, paramB: String,
    paramC: String)

//it also works for lambda
val fieldExample =
        LongNameClass { paramA,
                        paramB,
                        paramC ->
            ClassB(paramA, paramB, paramC)
        }

Non valid examples

class ClassA(paramA: String, paramB: String,
    paramC: String)

class ClassA(
    paramA: String, paramB: String, 
  paramC: String)

val fieldExample =
        LongNameClass { paramA,
            paramB,
            paramC ->
            ClassB(paramA, paramB, paramC)
        }

ParametersOnSeparateLinesRule takes care about each parameter

Parameters should:

  • either occupy only one line
  • either each parameter should be placed on new line

Exception: this rule doesn't take case of lambda (see example below)

Valid

class ClassA(paramA: String, paramB: String, paramC: String)

class ClassA(
    paramA: String,
    paramB: String,
    paramC: String)

class ClassA(
    paramA: String,
    paramB: String,
    paramC: String
)

As you see it doesn't care about enclosing brace.

Non Valid

class ClassA(paramA: String, paramB: String,
             paramC: String)

class ClassA(paramA: String,
             paramB: String,
             paramC: String)

class ClassA(
    paramA: String, paramB: String,
    paramC: String
)

Exception for lambda

val fieldExample =
        LongNameClass { paramA,
                        paramB,
                        paramC ->
            ClassB(paramA, paramB, paramC)
        }

this example is not enforced.

Final notes
Final brace ) is not enforced and lambda is not supported. I did it by purpose, in order to reduce scope of changes. I would prefer to do it in separate PRs.

@shyiko let me know what you think.

@MyDogTom
Copy link
Contributor Author

@vanniktech good question.
I would say yes. The initial rationale for me is: Having each parameter on separate line improves readability. In case of multiple parameters per line, I have to "parse" that line when I read it, in order to find all parameters.
And for me same rationale applies for classes, functions etc.

@shyiko what do you think?

@vanniktech
Copy link
Contributor

I would say yes. The initial rationale for me is: Having each parameter on separate line improves readability. In case of multiple parameters per line, I have to "parse" that line when I read it, in order to find all parameters.
And for me same rationale applies for classes, functions etc.

This becomes really verbose really quickly especially when you have a large function call. I'd like to have a flag for this so that you can either enable or disable this for parameters.

@MyDogTom
Copy link
Contributor Author

Sorry, for my previous "too quick" response. Actually, this is aligned with
https://android.github.io/kotlin-guides/style.html#functions

When a function signature does not fit on a single line, break each parameter declaration onto its own line.

@vanniktech
Copy link
Contributor

I like the way it'll be done in kotlinpoet - square/kotlinpoet#281

@MyDogTom
Copy link
Contributor Author

@vanniktech I'm not sure what you mean exactly. Decide wrap or not according to line length? If yes, we should definitely do that. But I would prefer to do it in separate PR. Just to keep current one smaller and merge it earlier.

@shyiko I'm looking forward for your feedback :)

@vanniktech
Copy link
Contributor

Then let's do it in a different PR.

@shyiko
Copy link
Collaborator

shyiko commented Dec 27, 2017

@MyDogTom (just so we are all on the same page) I plan to merge this + #126 (with a few adjustments related to braces) right after the New Year's Eve.

@MyDogTom
Copy link
Contributor Author

@shyiko 👍

@MyDogTom
Copy link
Contributor Author

These changes are included into #137

@MyDogTom MyDogTom closed this Jan 13, 2018
@MyDogTom MyDogTom deleted the mydogtom/indention-rule-align-parameters branch January 13, 2018 20:33
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.

3 participants