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

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

… 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
@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
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 this pull request may close these issues.

Support for globally disabling a rule
6 participants