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

Use cli param value as a source of globally disabled rules #533

Closed
idntfy opened this issue Jul 17, 2019 · 8 comments · Fixed by #534
Closed

Use cli param value as a source of globally disabled rules #533

idntfy opened this issue Jul 17, 2019 · 8 comments · Fixed by #534
Milestone

Comments

@idntfy
Copy link

idntfy commented Jul 17, 2019

Context

  • Add support for disabled_rules property in .editorconfig for globally disabling rules #503 - adds support for globally disabled rules through editorconfig

    # 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

  • Wildcard imports #48 - summarising, is a very long discussion about abusing the authority by enabling no-wildcard-import by default in the tool we all love.

This issue is an attempt to find a pragmatic consensus.

Although it would seem that #503 could resolve the situation around #48 but it still hurts the wide adoption of the tool.

On a scale it's really hard to adopt now when it's enabled. We have both big number of projects and even more bigger number of developers. Committing .editorconfig to all the projects or asking everyone to call --apply-to-idea-project are just no way to go, because it cannot be centralized thus hardly manageable.

The Proposal 1 (UPD: this was chosen to implement now)

Introduce a cli param value of which would be used as a source to the functionality already introduced in #503 - so basically it's nothing new.

In this form it can be configured widely even across many projects, e.g. if parent-pom or some common maven/gradle plugin is used, without committing .editorconfig to all the projects and without asking an every single developer to change default idea settings by calling --apply-to-idea-project.

The Proposal 2 (UPD: #548 was created to discuss further)

Create several sets of code styles:

  • default, Jetbrains-based on official kotlin coding conventions and intelliJ idea default settings - this one is not supported now.
  • android-specific, which would match google's recommendation for android development - it seems ktlint tends to support this standard.

Make it switchable through cli param. Thus all the parties - android developers and the rest - would use what they need.


Bottomline: while 2 could be too extreme, 1 is just a simple addition to the functionality that already exists.

@idntfy idntfy mentioned this issue Jul 17, 2019
@shashachu
Copy link
Contributor

An --android CLI param actually already exists, but it's not being used in this capacity. I don't think your proposal to move the no wildcards rule into and android ruleset is a bad, as it does exist only in the Android Kotlin styleguide. I admit that as an Android developer I'm biased towards that direction, but I don't have a good pulse on how non-Android Kotlin developers work. The reason I'm hesitant is that it opens the door to a lot of arguing about whether a rule should be android-only, and as far as I know, the no wildcard imports rule would be the only rule in the android ruleset. Let me give this some thought.

@JakeWharton
Copy link
Contributor

JakeWharton commented Jul 17, 2019

The Google Kotlin style guide (which encompasses all Kotlin code written at Google, not just Android code) also has this rule and they are ktlint consumers.

It's currently a copy of the Google Android Kotlin style guide with some tweaks (such as 2 spaces instead of 4, praise be). I hope one day to invert that dependency and have a publicly-published Google Kotlin style guide with the Google Android Kotlin style guide being only tweaks to it (4 space instead of 2, booooooooo).

@idntfy
Copy link
Author

idntfy commented Jul 17, 2019

The reason I'm hesitant is that it opens the door to a lot of arguing about whether a rule should be android-only

Call the potatoes default and strict. The ones who disagree would be glad to use something that's called strict, the ones who prefer default ruleset would be glad to use the default one.

If not, the first proposal seems to be pretty simple to implement which would be satisfactory as well.

@idntfy
Copy link
Author

idntfy commented Jul 17, 2019

After a bit of thinking - I think we need to proceed with the first proposal: just adding a cli param to do the same as #503 does.

I'll describe the thought process: since the notion of default changes with the ruleset proposal, the current consumers will need to switch to the behaviour they got used to. I imagine you need to maintain your current audience happy, only new consumers are not happy with this no-wildcards enabled. Hence, the strict one becomes default, and the other ruleset just weakens rules. However, for this it would be more flexible to provide the above-mentioned cli param.

@shashachu
Copy link
Contributor

shashachu commented Jul 17, 2019

After a bit of thinking - I think we need to proceed with the first proposal: just adding a cli param to do the same as #503 does.

I'll describe the thought process: since the notion of default changes with the ruleset proposal, the current consumers will need to switch to the behaviour they got used to. I imagine you need to maintain your current audience happy, only new consumers are not happy with this no-wildcards enabled. Hence, the strict one becomes default, and the other ruleset just weakens rules. However, for this it would be more flexible to provide the above-mentioned cli param.

Makes sense. Moving rules into --android is, like you said, somewhat of a backwards-incompatible change, and perhaps something worthy of a larger discussion.

I'll put up a PR for adding --disabled_rules as a CLI flag and push it with the already-planned 0.34.1 release, due the end of this week.

shashachu added a commit to shashachu/ktlint that referenced this issue Jul 17, 2019
Fixes pinterest#533

Note: command line flag will override `.editorconfig`
@shashachu shashachu added this to the 0.34.1 milestone Jul 17, 2019
@idntfy
Copy link
Author

idntfy commented Jul 17, 2019

Thank you for a quick turnaround @shashachu 🎉

shashachu added a commit that referenced this issue Jul 17, 2019
Fixes #533

Note: command line flag will override `.editorconfig`
@shashachu
Copy link
Contributor

I'll wait a couple days to make sure there aren't any other issues we need to patch for 0.34.0 then put up a new release.

sowmyav24 pushed a commit to sowmyav24/ktlint that referenced this issue Jul 18, 2019
Fixes pinterest#533

Note: command line flag will override `.editorconfig`
@shashachu
Copy link
Contributor

@idntfy I just released 0.34.1 with the new flag.

orchestr7 pushed a commit to saveourtool/diktat that referenced this issue Jun 29, 2020
Fixes pinterest/ktlint#533

Note: command line flag will override `.editorconfig`
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants