ktlint icon indicating copy to clipboard operation
ktlint copied to clipboard

Block ktlint-disable directive not working

Open JoseAlcerreca opened this issue 4 years ago • 5 comments

Version: 0.39.0## Expected Behavior

/* ktlint-disable indent */ should disable the indent rule from that point on.

Observed Behavior

Still getting the ktlint errors that the directive is supposed to disable

Steps to Reproduce

ktlint is used in AndroidX and we want to disable some checks (they're snippets in documentation). For example: https://android-review.googlesource.com/c/platform/frameworks/support/+/1497420/6/compose/integration-tests/docs-snippets/src/main/java/androidx/compose/ui/docs/tutorial/Tutorial.kt

The ktlint config can be found in https://cs.android.com/androidx/platform/frameworks/support/+/androidx-master-dev:buildSrc/src/main/kotlin/androidx/build/Ktlint.kt?q=ktlint.kt&ss=androidx

However, code after /* ktlint-disable indent */ is still breaking the build:

/* ktlint-disable indent */
...
            Text("Text", Modifier.constrainAs(text) {
                top.linkTo(button.bottom, margin = 16.dp)
            })

shows

docs-snippets/src/main/java/androidx/compose/ui/docs/layout/Layout.kt:174:13: Missing newline before ")" (indent)

My workaround is to add // disable-ktlint at the top of the file and, for some reason, I also need to add /* ktlint-disable no-unused-imports */, otherwise every import triggers an error. It looks like a different, unrelated problem.

JoseAlcerreca avatar Nov 17 '20 15:11 JoseAlcerreca

yea, that's unfortunately not possible at the moment as the indent rule runs all or nothing:

https://github.com/pinterest/ktlint/blob/b4f084454fbe9654ba56d1b354c8a98d7baba01a/ktlint-ruleset-standard/src/main/kotlin/com/pinterest/ktlint/ruleset/standard/IndentationRule.kt#L96-L99

we are planning to change this in the upcoming release and make the rule more granular so you can disable node-specific rules.

the 2nd issue sounds like a bug indeed, but let's see how it behaves after the split. (btw, you could update your ktlint config as the paren-spacing and import-ordering rules were already fixed)

romtsn avatar Nov 17 '20 16:11 romtsn

@liutikas for the ktlint config.

Thanks Roman, I've added the "all" directive to the affected files for now, pointing to this issue. Is there a better issue to point to? https://android-review.googlesource.com/c/platform/frameworks/support/+/1497420/7/compose/integration-tests/docs-snippets/src/main/java/androidx/compose/integration/docs/tutorial/Tutorial.kt#1

I don't think the file scope for disable directives is documented, is it? In any case, putting it on the first line can cause problems with tools checking for copyright notices, so I suggest adding something more flexible :)

JoseAlcerreca avatar Nov 17 '20 18:11 JoseAlcerreca

yeah, this issue should be fine to point to. let's hope that you would not need to disable rules for the entire file anymore, but I agree we should think of something more flexible and straightforward ;)

romtsn avatar Nov 17 '20 21:11 romtsn

Hi @romtsn,

I just wanted to add that this feature would be quite valuable for the teams at my company as well. Therefore I wanted to ask, - whether it is already planned to implement this feature or if work is already being done?

  • how complex you expect the implementation to be?

Depending on your responses I might be able to invest some time in order to support this feature request.

Also if adding this comment to this existing issue is inappropriate, please let me know.

Thanks in advance! 🙏

aablsk avatar Sep 03 '21 09:09 aablsk

I just wanted to add that this feature would be quite valuable for the teams at my company as well. Therefore I wanted to ask, - whether it is already planned to implement this feature or if work is already being done?

  • how complex you expect the implementation to be?

Depending on your responses I might be able to invest some time in order to support this feature request.

The indent-rule is one of the most complex rules in ktlint. A first step would be to split this rule into separate rules regarding line wrapping and indent fixing. For this, we have to await https://github.com/pinterest/ktlint/pull/1230. The next step after that would be cleaning up code and merging the expectedIndex variable into the IndentContext. After that it might be more clear what follow up steps have to be taken.

@aablsk Help is appreciated. It might be an idea to first get a bit acquainted with this rule by fixing other indentation problems which are reported in the issue tracker.

paul-dingemans avatar Dec 23 '21 20:12 paul-dingemans

Fixed by #1547

paul-dingemans avatar Aug 16 '22 18:08 paul-dingemans