Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Questions about IndentionRule #233

Closed
MyDogTom opened this issue Jun 20, 2018 · 3 comments
Closed

Questions about IndentionRule #233

MyDogTom opened this issue Jun 20, 2018 · 3 comments

Comments

@MyDogTom
Copy link
Contributor

MyDogTom commented Jun 20, 2018

Hi @shyiko . I finally have some time and would like to proceed with #141 . I didn't follow the project since 0.15.1. I have some questions.

Question 1.
While refreshing my memory, I noticed that IntentRule doesn't support auto correction anymore. Auto correction was merged into master as part of #137 , but now I cannot find any traces of that change. Any reason for that? Do you see auto correction for IndentRule as welcomed change?

Question 2
There is a TODO in IndentationRule

calculating indent based on the previous line value is wrong (see IndentationRule.testLint)

Could you elaborate a bit what is wrong with that?

Question 3
Android style guide has definition for "continuation indent", but doesn't have any cases when to use it. Kotlin coding conventions doesn't even have definition of "continuation indent". IndentionRule uses "greater common divider" when choose indent size. Basically, it means that regular indent will be always selected.
I would make it easier and always use "regular indent". Unless specific case, when exactly "continuation indent" should be used, pops up. What do you think?

@MyDogTom MyDogTom changed the title Question: IndentionRule and auto correction Questions about IndentionRule Jun 20, 2018
@MyDogTom
Copy link
Contributor Author

I updated description with two additional questions. @shyiko Let me know if you prefer separate issues instead.

@shyiko
Copy link
Collaborator

shyiko commented Jun 21, 2018

Hey @MyDogTom. Welcome back!

Auto correction was merged into master as part of #137 , but now I cannot find any traces of that change. Any reason for that?

I had to go with an alternative implementation (ParameterListWrappingRule) as I was getting a bunch of issues with that rule. Original ClassAndFunctionHeaderFormatRule is still available in the develop branch.

Do you see auto correction for IndentRule as welcomed change?

Absolutely. It's one of the most requested features.

Could you elaborate a bit what is wrong with that?

$ printf "val a =\n b\n  .c()\n" | ktlint --stdin
<text>:2:1: Unexpected indentation (1) (it should be 4)
<text>:3:1: Unexpected indentation (2) (it should be 5)

$ printf "val a =\n b\n    .c()\n" | ktlint --stdin
<text>:2:1: Unexpected indentation (1) (it should be 4)
<text>:3:1: Unexpected indentation (4) (it should be 5)

In short, if the previous line is miss-aligned the "expected" indentation value is not calculated properly. There multiple ways to fix this, the best being rewrite IndentationRule to calculate/auto-correct everything in one pass.

I would make it easier and always use "regular indent". Unless specific case, when exactly "continuation indent" should be used, pops up. What do you think?

I agree. Let's avoid any "continuation indent" logic if we can help it.

@MyDogTom
Copy link
Contributor Author

I had to go with an alternative implementation (ParameterListWrappingRule) as I was getting a bunch of issues with that rule.

#137 contains ClassAndFunctionHeaderFormatRule (which is ParameterListWrappingRule not) and auto correction for IndentRule. Anyway, I bring it back in #239

In short, if the previous line is miss-aligned the "expected" indentation value is not calculated properly.

That's why, in first PR I displayed error with relative indent and not with absolute (see commit]. Afterwards, it was changed to absolute. I'll create a separate ticket with my proposal about error message.
As for "one pass", I agree that it's a good approach, but for different reason - it should be more efficient (performance wise).
Advantage of current approach is that it's much simpler.

I agree. Let's avoid any "continuation indent" logic if we can help it.

During my tests, I've found at least one case when "continuation indent" should be used. I'll create a separate ticket to discuss it.

@shyiko thanks, for your answer. I'm closing it, since further discussion will be in separate issues (to make it more focused).

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants