config/default: Stop disabling all cops by default #117
Conversation
- This is _incredibly_ confusing behaviour, it turns out: github/code-scanning#7361, github/github#241104. - People have been adding cops to `github/github` and they've not been running because with `DisabledByDefault`, nothing runs unless there's an explicit config stanza or it's already declared upstream in this gem. - Since `github/github`'s RuboCop config also had `DisabledByDefault` and we wanted to set `EnabledByDefault` there to alleviate the above confusion, we couldn't: "cops cannot be both enabled by default and disabled by default". - This flips `DisabledByDefault: true` to `EnabledByDefault: true`, so that RuboCop config is easier to understand and the behaviour is more in line with what normal users expect. - I understand that `DisabledByDefault` was set to avoid RuboCop versions changing which have new cops set up, or existing cop config changes, that cause linting violations everywhere, but it's way worse for understanding how things work, as the issues/PRs above reveal! If there's something that causes a lot of violations, we can disable it either here or in the downstream app codebase. This shouldn't be too much of an issue because bumping this gem version or a RuboCop version requires an explicit PR/approval/deploy workflow with CI, so we can catch the really noisy things.
|
Ah, good, now I have to go through all the new rules (at least the ones I can tell that showed up problems in this codebase) and either fix them here or disable them. |
- Since `EnabledByDefault: true`, we can slim this down to only the ones we want to disable either because the styleguide doesn't like the rules, or because the styleguide has no opinion yet. - I added some more `Enabled: false` here for cops that are failing in the code in this gem and others, based on the information or lack of in our styleguide.
- This works because the RuboCop rule for line length isn't enabled by default (it never was in this gem for some reason). Some of these lines are pretty long now!
- Either the styleguide has no opinion on them (yet), or it doesn't like them, or they'd be quite disruptive.
- These either seemed arbitrary or had no accompanying styleguide rule that we've written yet.
|
The code in this repo should now pass RuboCop. There are some arbitrary, and some styleguide-motivated
|
config/default.yml
Outdated
| @@ -3,182 +3,68 @@ require: | |||
| - rubocop-performance | |||
|
|
|||
| AllCops: | |||
| DisabledByDefault: true | |||
| EnabledByDefault: true | |||
There was a problem hiding this comment.
I think that this repository should not declare either DisabledByDefault nor EnabledByDefault and leave that to the downstream inheritor of this configuration.
That way this ruleset can be completely verbose of "these are the baseline rules", and inheriting projects can choose to further inherit all of the Rubocop rules however they want.
I'm not 100% sure on this, I think it will have the same effect in this repository, but will effect downstream repositories.
There was a problem hiding this comment.
Relevant to our chat earlier, maybe setting DisabledByDefault was so that whichever team first made this gem would be an authority on Ruby at GitHub, and other downstream consumers diverged for their own code style requirements. Is that maybe why it's not 100% compatible with the styleguide now?
There was a problem hiding this comment.
Oops, I shouldn't have written "this repository"; I specifically meant config/default.yml shouldn't define neither DisabledByDefault nor EnabledByDefault.
The rules for this repository, .rubocop.yml should probably have EnabledByDefault.
There was a problem hiding this comment.
Riiiight. I see. Makes sense, I think?
There was a problem hiding this comment.
So, to recap:
- You'd like
.rubocop.ymlin this repository (the RuboCop config for the code in this gem) to setEnabledByDefault. - You would like
config/default.yml(which all of the downstream consumers of this gem inherit their rules from) to not set eitherEnabledbyDefaultorDisabledByDefault? That means that it's just using the default RuboCop behaviour, which is probably a safe bet.
I'm not sure what benefit this first thing has, since the RuboCop defaults are similar enough?
- This doesn't go back to `DisabledByDefault: true`, it takes a neutral stance to match whatever RuboCop's defaults are for enabling (or not) cops. Then it's up to the maintainers of this gem (us, the Hubber community, I guess) to decide the settings for the rules to include, and/or it's up to the downstream consumers of this gem to decide whether they want all the rules or just some of the rules as they are defined in here. - I hope I'm understanding this correctly? I did go and read the sourc RuboCop configuration file for this: https://github.com/rubocop/rubocop/blob/d1270c468cdb9a211229e36aef8d568ae3bfe607/config/default.yml#L91-L102
|
I guess now we get to decide who makes decisions about enabling new cops? |
|
We could automatically opt-in, and for every new RuboCop version we update to we could say "how noisy is this to autofix" and make a decision then (ie, in the downstream consumer project), and then eventually bubble those choices up to here? |
| Bundler/DuplicatedGem: | ||
| Enabled: true | ||
|
|
||
| Bundler/OrderedGems: | ||
| Enabled: true | ||
|
|
||
| GitHub/InsecureHashAlgorithm: | ||
| Enabled: true | ||
| Gemspec/DependencyVersion: | ||
| Enabled: false |
There was a problem hiding this comment.
For some of these new ones that fail RuboCop in the code in this gem: should we put them in here, the global config for the gem and the one that all its downstream consumers use, or should we put them in .rubocop.yml inside this repo only?
I guess I'm now concerned, after @bensheldon's comments, about this gem's config not being a reflection of the styleguide? Who gets to make decisions on what we include in the styleguide, and hence who gets to "control" what sensible default RuboCop configuration we send out into the world?
|
I also don't want to conflate our two projects here. The engineering initiative that I'm working on was focused on To some extent it feels like wading in here is too much. But we do need to remove |
|
@issyl0 I hope this is a helpful set of suggestions to move this PR forward:
|
|
Very helpful, thanks! |
|
I'll close this and open another set of PRs as this one has got long and messy. But the conversation is still super valuable! |
|
@issyl0 great! One other suggestion: if, during correcting, you discover there are some rules that you don't like, add them to this project's Then separately, we can decide which things should go into I think that'll keep the core/necessary changes flowing through. |
- The `DisabledByDefault` config option made it so that "all cops in the default configuration are disabled, and only cops in user configuration are enabled", which makes cops "opt-in rather than opt-out" (source: https://github.com/rubocop/rubocop/blob/d1270c468cdb9a211229e36aef8d568ae3bfe607/config/default.yml#L91-L102). - It's not immediately obvious that this gem has this config option, and it's used in a lot of GitHub Ruby projects. This means that people may write new, custom cops in downstream projects, with no configuration in `.rubocop.yml`, and then they never run because all cops are disabled unless explicitly enabled or otherwise configured with `Include`/`Exclude`. - This change is a follow-up to the great conversations had in #117, and makes this gem take a more neutral stance to match whatever RuboCop's defaults are for enabling (or not) cops. Then it's up to the maintainers of this gem and the deciders of our styleguide to pick the settings for the rules to include, and/or it's up to the downstream consumers of this gem to decide whether they want all the rules or just some of the rules as they are defined in here.
This is incredibly confusing behaviour, it turns out: https://github.com/github/code-scanning/issues/7361, https://github.com/github/github/pull/241104.
People have been adding cops to
github/githuband they've not been running because withDisabledByDefault, nothing runs unless there's an explicit config stanza or it's already declared upstream in this gem.Since
github/github's RuboCop config also hadDisabledByDefaultand we wanted to setEnabledByDefaultthere to alleviate the above confusion, we couldn't: "cops cannot be both enabled by default and disabled by default".This
flipsfalls back to the RuboCop default enable/disable settings, so that RuboCop config is easier to understand and the behaviour is more in line with what normal users expect.DisabledByDefault: truetoEnabledByDefault: trueI understand that
DisabledByDefaultwas set to avoid RuboCop versions changing which have new cops set up, or existing cop config changes, that cause linting violations everywhere, but it's way worse for understanding how things work, as the issues/PRs above reveal! If there's something that causes a lot of violations, we can disable it either here or in the downstream app codebase. This shouldn't be too much of an issue because bumping this gem version or a RuboCop version requires an explicit PR/approval/deploy workflow with CI, so we can catch the really noisy things.Part of https://github.com/github/code-scanning/issues/7361.