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

disabled-rules for ktlint in .editorconfig is ignored #112

Closed
dadadom opened this issue Aug 6, 2019 · 13 comments · Fixed by #113
Closed

disabled-rules for ktlint in .editorconfig is ignored #112

dadadom opened this issue Aug 6, 2019 · 13 comments · Fixed by #113

Comments

@dadadom
Copy link

dadadom commented Aug 6, 2019

Since 0.34, ktlint supports disabling rules globally with a setting in the .editorconfig file:

# Comma-separated list of rules to disable (Since 0.34.0)
# Note that rules in any ruleset other than the standard ruleset will need to be prefixed 
# by the ruleset identifier.
disabled_rules=no-wildcard-imports,experimental:annotation,my-custom-ruleset:my-custom-rule

(see https://github.com/pinterest/ktlint#custom-editorconfig-properties)

It seems like kotlinter does not pick up that configuration setting and it has to be repeated in the corresponding kotlinter configuration that was introduced in version 2.0.0.

Am I missing something or is this duplication necessary?

@jeremymailen
Copy link
Owner

Thanks for the report -- it looks like this broken between 2.0.0 and 2.1.0! Embarrassed I didn't catch it. Let me investigate and fix.

@jeremymailen
Copy link
Owner

Ok, give 2.1.1 a try.

I suspect I'm going to have to address the same issue for other properties that can be in both places such as indentSize.

@dadadom
Copy link
Author

dadadom commented Aug 7, 2019

Thank you for the new release. With this one, I do not get the errors any more, BUT it also doesn't pick up the other rules/restrictions from .editorconfig.

So, my setup is like this:
.editorconfig (relevant lines only):

[*]
max_line_length             = 120

[*.{kt,kts}]
disabled_rules              = max-line-length,import-ordering

With kotlinter 2.1.0:

Lint error > src/main/kotlin/.../ConfigurationBuilder.kt:3:1: [import-ordering] Imports must be ordered in lexicographic order without any empty lines in-between
Lint error > src/main/kotlin/.../ConfigurationBuilder.kt:19:1: [max-line-length] Exceeded max line length (120)

So, this is the case I was describing initially: kotlinter ignores the disabled_rules. And initial testing with 2.1.1 also seems to have confirmed the fix because the run yields no errors.

But I did the counter-check with this .editorconfig:

[*]
max_line_length             = 120

And with kotlinter 2.1.1:

BUILD SUCCESSFUL in 4s

So it seems like the original max_line_length = 120 rule doesn't get picked up at all now?

@jeremymailen jeremymailen reopened this Aug 8, 2019
@jeremymailen
Copy link
Owner

Do you still have this line in .editorconfig?

disabled_rules              = max-line-length,import-ordering

If not, can you try gradle clean lintKotlin and see if it picks up the line length violations then?
That's the only way I could reproduce the bug. Since gradle isn't aware of the .editorconfig file as build configuration right now there's a wrinkle/bug where if you get a successful gradle lintKotlin run and then don't change any source code and only adjust rules in the .editorconfig file, gradle skips running the lintKotlin task and thinks it can use the cached result :/. Sort of a rare situation though.

@jeremymailen
Copy link
Owner

jeremymailen commented Aug 8, 2019

I am aware that accepting indent_size from .editorconfig is still broken. Going to have to adjust the strategy for using the .editorconfig file and actually read it to combine with values from the kotlinter extension to fix this bug properly. I can probably fix the build caching wrinkle too in the process.

@dadadom
Copy link
Author

dadadom commented Aug 8, 2019

Thanks for the "workaround", doing an explicit gradle clean lintKotlin actually does the trick and from my testing, kotlinter 2.1.1 does pick up the configurations correctly!

@dadadom dadadom closed this as completed Aug 8, 2019
@lwasyl
Copy link
Contributor

lwasyl commented Aug 15, 2019

Still happens to me with 2.1.1 though :\

[*.{kt, kts, gradle}]
indent_size = 4
continuation_indent_size = 4
insert_final_newline = true
max_line_length = 140
disabled_rules = import-ordering

I'm still seeing errors from import ordering rule. Excluding them in plugin extension works, using .editorconfig doesn't (I don't have any other exclusions in the extension)
Specifying disabled_rules in [*.{kt, kts}] doesn't fix this either. I cleaned the project and ran the task with --rerun-tasks --no-build-cache flags.

@jeremymailen
Copy link
Owner

@lwasyl it looks like .editorconfig doesn't like spaces in the file type selector. I was able to get it working with:

[*.{kt,kts,gradle}]
disabled_rules = import-ordering

@mindhaq
Copy link

mindhaq commented Sep 23, 2019

@jeremymailen guess how IntelliJ reformats that file if you let it. Right, with spaces between the commas in the file type selector.

@jeremymailen
Copy link
Owner

Yes 😞 that is a bummer @mindhaq. The editorconfig parsing functionality comes from ktlint in this case. They did switch from a custom implementation to using ec4j (pinterest/ktlint#561). It's not in a release yet, but I imagine once we can upgrade to that it will fix the quirk with whitespace breaking the section.

@mindhaq
Copy link

mindhaq commented Sep 26, 2019

Maybe we need an .editorconfig.editorconfig so that IntelliJ is formatting our .editorconfig correctly.

@gigaSproule
Copy link

This can be solved by unchecking After comma under File | Settings | Editor | Code Style | EditorConfig.

@ulaserdegor
Copy link

This can be solved by unchecking After comma under File | Settings | Editor | Code Style | EditorConfig.

this resolve! thank you

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 a pull request may close this issue.

6 participants