Implement .ktlintignore for globally disabling rules #459

shashachu · 2019-06-06T21:52:35Z

Fixes #208

Added support for .ktlintignore file - takes a list of rule Ids to disable (ignores comments)
Currently only supports globally disabling rules, but added some plumbing so that it's easier to support per-file/per-path disabling, or multiple .ktlintignore files
Re-enabled NoWildcardImports, PackageNameRule
Un-commented AnnotationRule, MultiLineIfElseRule, and NoItParamInMultilineLambdaRule, and moved disabling into .ktlintignore

Fixes pinterest#208 * Added support for .ktlintignore file - takes a list of rule Ids to disable (ignores comments) * Currently only supports globally disabling rules, but added some plumbing so that it's easier to support per-file/per-path disabling, or multiple .ktlintignore files * Re-enabled NoWildcardImports, PackageNameRule * Un-commented AnnotationRule, MultiLineIfElseRule, and NoItParamInMultilineLambdaRule, and moved disabling into .ktlintignore

shashachu · 2019-06-06T21:53:03Z

.ktlintignore

@@ -0,0 +1,7 @@
+// disabled ("./mvnw clean verify" fails with "Internal Error")


This file is kind of goofy, but I liked the idea of allowing comments so that we can explain why a rule is disabled

shashachu · 2019-06-06T21:53:51Z

ktlint/src/main/kotlin/com/pinterest/ktlint/Main.kt

+                ) +
+                cliUserData +
+                // For now, use a global list
+                ("disabled_rules" to disabledRules.joinToString(",")) +


Here is where we'd handle logic around a .ktlintignore file that's formatted more like an .editorconfig file.

shashachu · 2019-06-06T21:54:28Z

Open for suggestions on good ways to test this.

shashachu · 2019-06-06T21:58:48Z

@JLLeitschuh @jaredsburrows @jeremymailen thoughts?

jeremymailen

I like the idea of introducing this feature!

jeremymailen · 2019-06-07T07:03:05Z

ktlint/src/main/kotlin/com/pinterest/ktlint/Main.kt

@@ -300,6 +307,10 @@ object Main {
            exitProcess(0)
        }
        val start = System.currentTimeMillis()
+
+        // Parse the .ktlintignore file
+        parseKtlintIgnore(ktlintIgnorePath ?: ".")


It would be useful to put support classes in the core module to locate any .ktlintignore files and parse them into disabled rules userdata. This would allow tools not invoking the CLI portion of ktlint to have an easy way to support .ktlintignore exactly the same way.

sounds good. You actually know much more about the internals of ktlint than I do... 🤦‍♀

jeremymailen · 2019-06-07T07:09:57Z

ktlint/src/main/kotlin/com/pinterest/ktlint/Main.kt

@@ -234,6 +235,12 @@ object Main {
    )
    private var experimental: Boolean = false

+    @Option(names = arrayOf("--ktlintignore"), description = arrayOf("Path to .ktlintignore (defaults to working dir)"))


Would they pass in a path or would it happen by convention such as working directory plus home directory similar to .gitignore? I guess global prefs make less sense here -- possibly at each package level? Although I'm not sure if it's easy to support or even worth it.

For kotlinter-gradle I suppose I'd start by saying the file had to be in the gradle project root. Of course ktlint CLI is agnostic to the idea of a project.

If you look a bit further down, it does default to ".".

And yes, I was unsure whether we want/need to support multiple ignore/config files. I can certainly imagine a scenario (eg codegen files) but I'm unsure how common it would be. It seems like the biggest ask is for global disabling.

I originally considered a .ktlintconfig with the same syntax as .editorconfig so you could easily disable rules on a per-path basis but thought maybe the simpler approach was fine for now.

I would agree with @shashachu, that it should be possible to add additionally rules to exclude in subprojects and .ktlintconfig should behave like .editorconfig.

jeremymailen · 2019-06-07T07:17:47Z

.ktlintignore

@@ -0,0 +1,7 @@
+// disabled ("./mvnw clean verify" fails with "Internal Error")


nit: .xyzignore files are conventionally lists of file expressions to ignore. What if it was .ktlintconfig and perhaps in something like toml?

Yes - it's goofy. Probably a better format is something like .editorconfig:

[*] disable_rules=no-wildcard-imports [**/library/*] disable_rules=no-wildcard-imports,package-name

Yes - it's goofy. Probably a better format is something like .editorconfig:

yes, this format of the file looks better and will allow in the future add more ktlint specific configuration here.

romtsn · 2019-06-07T09:28:52Z

ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/KtLint.kt

@@ -254,6 +257,10 @@ object KtLint {
        }
    }

+    private fun filterDisabledRules(rootNode: ASTNode, fqRuleId: String): Boolean {
+        return rootNode.getUserData(DISABLED_RULES)?.contains(fqRuleId) == false


Note: this will currently work only with the standard ruleset. If you have an experimental one (or any other custom ruleset), you have to specify the rule with the prefix inside .ktlintignore (as you can see it's being added to the rule id at the line 207). So something like experimental:indent - I guess this should be mentioned in the docs or something :) Or we could change this to any { fqRuleId.contains(it) }, if we want to be independent of the ruleset (as I assume rule ids most of the time are unique)

That's correct. I'd rather require fully namespaced rules than unexpectedly disabling a rule with a name collision. We would definitely update the docs if we move forward with this.

JLLeitschuh · 2019-06-07T17:01:02Z

CC: @Tapchicoma

jaredsburrows · 2019-06-07T21:01:49Z

This project already uses .editorconfig, why not add this functionality to .editorconfig? This way we do not have to have another file to configure ktlint.

shashachu · 2019-06-07T21:41:26Z

This project already uses .editorconfig, why not add this functionality to .editorconfig? This way we do not have to have another file to configure ktlint.

It felt like an abuse of .editorconfig. I suppose disabling lint rules falls loosely under 'coding style', but it doesn't really seem like it fits the spirit of what an .editorconfig is supposed to do. I'm more on the side of it being a separate file, but it's not a dealbreaker for me.

bethcutler · 2019-06-07T22:00:51Z

Agreed that that would be an abuse of .editorconfig, which is a format that extends beyond just ktlint.

On the other hand, each new configuration file required for ktlint makes integration with other systems a bit more difficult. Would it make sense to add a new custom configuration file that could handle both the ignore rules and the editorconfig rules? Ideally, editorconfig would still be supported, but have fewer features.

shashachu · 2019-06-08T05:33:23Z

Agreed that that would be an abuse of .editorconfig, which is a format that extends beyond just ktlint.

On the other hand, each new configuration file required for ktlint makes integration with other systems a bit more difficult. Would it make sense to add a new custom configuration file that could handle both the ignore rules and the editorconfig rules? Ideally, editorconfig would still be supported, but have fewer features.

That would be fine as well. I do think that the ignore rules are currently the only ktlint-specific config, though.

Tapchicoma · 2019-06-08T19:32:04Z

ktlint/src/main/kotlin/com/pinterest/ktlint/Main.kt

+    /**
+     * Parses the .ktlintignore file, a list of ids of rules to globally disable. One rule per line.
+     */
+    private fun parseKtlintIgnore(path: String) {


I think, this function should repeat the logic of .editorconfig lookup - https://editorconfig.org/#file-location

Implementation:

ktlint/ktlint/src/main/kotlin/com/pinterest/ktlint/internal/EditorConfig.kt

Line 20 in 6f8bb56

override fun of(dir: Path) =

This would allow to define global ktlint rules in project root and have sub-project specific one, that will enhance/override global rules.

Tapchicoma · 2019-06-08T19:42:04Z

This project already uses .editorconfig, why not add this functionality to .editorconfig? This way we do not have to have another file to configure ktlint.

I agree with @jaredsburrows, from what I see - adding just custom property ktlint-disabled-rules to .editorconfig that falls under domain-specific-properties should be enough.

Though required change would be move detecting and parsing .editorconfig files to ktlint-core, so @jeremymailen plugin could also support this.

shashachu · 2019-06-10T17:11:29Z

Thanks for the feedback, everyone. I'll close this PR and open another one using either .editorconfig or a .ktlintconfig file.

jaredsburrows · 2019-06-11T02:50:04Z

Sounds good, thanks!

JorgeCastilloPrz · 2019-06-25T11:34:14Z

How's this going @shashachu ? Are there plans on opening that alternative PR? Let me know if you need any help, this is a feature I'm interested in.

shashachu · 2019-06-25T16:18:22Z

@JorgeCastilloPrz yes, there are still plans to do the PR; things have just been extremely busy. I have a PR in-progress locally, so I will aim to put it up by the end of the week.

JorgeCastilloPrz · 2019-06-25T16:53:22Z

Is there a place we can reach you for direct chat @shashachu ? Maybe Kotlinlang Slack? We can try to help on coding it if you're busy, we'd just need some indications since we're not familiar with the codebase.

shashachu · 2019-06-26T07:27:52Z

All, thanks for your patience. Pushed the updated editorconfig-based-PR here: #503

Please feel free to patch in and test locally.

@JorgeCastilloPrz I am working on getting a slack channel setup in the Pinterest slack team so that I can invite external contributors.

JorgeCastilloPrz · 2019-06-28T16:06:43Z

Thanks for your work on it! We'll keep a close eye on that PR :)

shashachu commented Jun 6, 2019

View reviewed changes

shashachu mentioned this pull request Jun 6, 2019

Rule: newline before "{" #458

Closed

shashachu changed the title ~~Implement .ktlintigore for globally disabling rules~~ Implement .ktlintignore for globally disabling rules Jun 6, 2019

jeremymailen reviewed Jun 7, 2019

View reviewed changes

romtsn reviewed Jun 7, 2019

View reviewed changes

Tapchicoma reviewed Jun 8, 2019

View reviewed changes

shashachu closed this Jun 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement .ktlintignore for globally disabling rules #459

Implement .ktlintignore for globally disabling rules #459

shashachu commented Jun 6, 2019

shashachu Jun 6, 2019

shashachu Jun 6, 2019

shashachu commented Jun 6, 2019

shashachu commented Jun 6, 2019

jeremymailen left a comment

jeremymailen Jun 7, 2019 •

edited

Loading

shashachu Jun 7, 2019

jeremymailen Jun 7, 2019

jeremymailen Jun 7, 2019

shashachu Jun 7, 2019

Tapchicoma Jun 8, 2019

jeremymailen Jun 7, 2019

shashachu Jun 7, 2019

Tapchicoma Jun 8, 2019

romtsn Jun 7, 2019

shashachu Jun 7, 2019

JLLeitschuh commented Jun 7, 2019

jaredsburrows commented Jun 7, 2019

shashachu commented Jun 7, 2019

bethcutler commented Jun 7, 2019

shashachu commented Jun 8, 2019

Tapchicoma Jun 8, 2019

Tapchicoma commented Jun 8, 2019

shashachu commented Jun 10, 2019

jaredsburrows commented Jun 11, 2019

JorgeCastilloPrz commented Jun 25, 2019

shashachu commented Jun 25, 2019

JorgeCastilloPrz commented Jun 25, 2019

shashachu commented Jun 26, 2019

JorgeCastilloPrz commented Jun 28, 2019 •

edited

Loading

		@@ -0,0 +1,7 @@
		// disabled ("./mvnw clean verify" fails with "Internal Error")

Implement .ktlintignore for globally disabling rules #459

Implement .ktlintignore for globally disabling rules #459

Conversation

shashachu commented Jun 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shashachu commented Jun 6, 2019

shashachu commented Jun 6, 2019

jeremymailen left a comment

Choose a reason for hiding this comment

jeremymailen Jun 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JLLeitschuh commented Jun 7, 2019

jaredsburrows commented Jun 7, 2019

shashachu commented Jun 7, 2019

bethcutler commented Jun 7, 2019

shashachu commented Jun 8, 2019

Choose a reason for hiding this comment

Tapchicoma commented Jun 8, 2019

shashachu commented Jun 10, 2019

jaredsburrows commented Jun 11, 2019

JorgeCastilloPrz commented Jun 25, 2019

shashachu commented Jun 25, 2019

JorgeCastilloPrz commented Jun 25, 2019

shashachu commented Jun 26, 2019

JorgeCastilloPrz commented Jun 28, 2019 • edited Loading

jeremymailen Jun 7, 2019 •

edited

Loading

JorgeCastilloPrz commented Jun 28, 2019 •

edited

Loading