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

Add support for disabled_rules property in .editorconfig for globally disabling rules #503

Merged
merged 7 commits into from
Jul 3, 2019

Conversation

shashachu
Copy link
Contributor

@shashachu shashachu commented Jun 26, 2019

Fixes #208

  • Takes in a comma-separated list of rule ids, with namespaces (e.g. experimental:indent)
  • Re-enabled NoWildcardImports, PackageNameRule
  • Un-commented AnnotationRule, MultiLineIfElseRule, and NoItParamInMultilineLambdaRule, and moved disabling into default .editorconfig
  • Moved .editorconfig parsing into ktlint-core so that plugins that don't invoke ktlint on the command line can take advantage of rule disabling
  • Also cleaned up params passed to lint and format

@@ -8,8 +8,8 @@ import java.util.Properties
import java.util.concurrent.CompletableFuture
import java.util.concurrent.ConcurrentHashMap

class EditorConfig private constructor (
val parent: EditorConfig?,
class EditorConfigInternal private constructor (
Copy link
Contributor Author

@shashachu shashachu Jun 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally open to better names for this class.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's wrong with just EditorConfig ;) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arturbosch there's already an EditorConfig interface inside of ktlint-core so if you want to use one or the other you need a lot of ugly namespacing. :) We can also rename that one, but I couldn't think of a name for that one!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, didn't know that.
I like to use a Defaultfor my implementations of an interface.
So maybe consider DefaultEditorConfig.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arturbosch but that's the thing - the internal one is actually not an implementation of the EditorConfig interface ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha correct! I don't have context about why it was implemented this way. I thought about naming the internal one ParsedEditorConfig or EditorConfigParser or something...

* @throws ParseException if text is not a valid Kotlin code
* @throws RuleExecutionException in case of internal failure caused by a bug in rule implementation
*/
fun lint(text: String, ruleSets: Iterable<RuleSet>, cb: (e: LintError) -> Unit) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed all the overrides and just pass in Params now.

.forEach { e -> params.cb(e, false) }
}

private fun userDataResolver(editorConfigPath: String?, debug: Boolean): (String?) -> Map<String, String> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved from Main.kt

@shashachu
Copy link
Contributor Author

@jaredsburrows @Tapchicoma @jeremymailen @JLLeitschuh @bethcutler Thoughts on this PR?

Copy link
Contributor

@jeremymailen jeremymailen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, looks great to me! A couple questions to make sure I understand how rule disabling is used, but structurally seems like it should work well with plugins that use ktlint.

text = "var foo",
ruleSets = listOf(RuleSet("standard", NoVarRule())),
cb = { e, _ -> add(e) },
userData = mapOf(("disabled_rules" to "no-var"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love the ability to pass in the disabled_rules as a regular userData param or via editor config 👍

# multiline-if-else - disabled until auto-correct is working properly
# (e.g. try formatting "if (true)\n return { _ ->\n _\n}")
# no-it-in-multiline-lambda - disabled until it's clear what to do in case of `import _.it`
disabled_rules=annotation,multiline-if-else,no-it-in-multiline-lambda
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a rule is not prefixed with anamespace: is it assumed to be in the standard set?

Copy link
Collaborator

@romtsn romtsn Jun 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, see this

shashachu added 4 commits July 2, 2019 21:59
… disabling rules

Fixes pinterest#208

* Takes in a comma-separated list of rule ids, with namespaces (e.g. `experimental:indent`)
* Re-enabled `NoWildcardImports`, `PackageNameRule`
* Un-commented `AnnotationRule`, `MultiLineIfElseRule`, and `NoItParamInMultilineLambdaRule`, and moved disabling into default `.editorconfig`
* Also cleaned up params passed to lint and format
@shashachu shashachu merged commit ba513d7 into pinterest:master Jul 3, 2019
@shashachu shashachu deleted the ktlintignore branch July 5, 2019 16:23
@JorgeCastilloPrz
Copy link

JorgeCastilloPrz commented Jul 8, 2019

Does it require some docs in README?

@shashachu
Copy link
Contributor Author

Yes; I like to wait to update the README until we're ready to release so it doesn't look like features are available that have not yet released.

@elenigen
Copy link

Do we know when is planned the next release of ktlin to include this new feature of allowing to disable rules?

@shashachu
Copy link
Contributor Author

Do we know when is planned the next release of ktlin to include this new feature of allowing to disable rules?

@elenigen I am targeting the end of this week.

sowmyav24 pushed a commit to sowmyav24/ktlint that referenced this pull request Jul 18, 2019
… disabling rules (pinterest#503)

* Add support for disabled_rules property to .editorconfig for globally disabling rules

Fixes pinterest#208

* Takes in a comma-separated list of rule ids, with namespaces (e.g. `experimental:indent`)
* Re-enabled `NoWildcardImports`, `PackageNameRule`
* Un-commented `AnnotationRule`, `MultiLineIfElseRule`, and `NoItParamInMultilineLambdaRule`, and moved disabling into default `.editorconfig`
* Also cleaned up params passed to lint and format
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for globally disabling a rule
6 participants