config/default: Unset DisabledByDefault: true
#119
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- 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 config applies only to this repo, it's not the global config.
- These rules seemed to make sense and were all autocorrectable: - Style/SymbolArray - Layout/EmptyLineAfterGuardClause - Style/MultipleComparison - Style/IfUnlessModifier (since we don't have line length limitations) - Style/EmptyCaseCondition - Style/TrailingUnderscoreVariable - Style/RedundantParentheses - Lint/UnusedBlockArgument - Style/NegatedIf - Style/StringConcatenation - Style/SafeNavigation - Layout/MultilineMethodCallIndentation - Layout/EmptyLineAfterMagicComment - Layout/SpaceInsideHashLiteralBraces - Layout/EmptyLines - Layout/EmptyLineBetweenDefs
issyl0
force-pushed
the
rubocop-default-enablement
branch
from
9a41f41
to
e36d683
Compare
|
This does everything that was suggested in #117 (comment). With the benefit of it being a smaller diff with fewer rules to configure, since |
- Now that this behaviour has changed in `rubocop-github`, our users might want to have finer-grained control of which cops they enable since they can't rely on this gem to include that option now (if they even noticed).
- Should probably write about the recent accessibility gem changes here, too, but that's not in this PR. Hence a generic 'unreleased' section.
- These rules come directly from RuboCop. They're configured `Enabled: false`
upstream, but it's worth being explicit in this gem since this guards
against RuboCop changing underneath us and causing linting changes that
either don't match our styleguide or we don't want to propagate to
users of this gem.
- This was generated by the following script (thanks to `@matthewd`),
where `show-cops.yml` is the output of
`bundle exec rubocop --show-cops`:
```ruby
require "yaml"
y = YAML.unsafe_load_file("show-cops.yml")
cfg = YAML.unsafe_load_file(".rubocop.yml")
off = y.keys.select { |k| y[k]["Enabled"] == false }
(off - cfg.keys).sort.each { |k| puts "#{k}:\n Enabled: false\n\n" }
```
- Next I'll do the same for `Enabled: true`, then in a follow-up commit
or PR (probably PR, at this point) I'll go through and link the
`Enabled: true` ones to their styleguide items in the GitHub styleguide.
- Previously these were disabled because of `DisableByDefault: true`.
Since we removed that config, all of these become enabled. That's
somewhat undesirable since when we ship the new version, downstream
consumers of this gem will have to make decisions to enable or disable
the cops. We should do this for them and make this gem the source of
truth matching the styleguide as far as possible. Of course, downstream
projects are free to tweak in their own `.rubocop.yml` for what matches
their project style, but we should attempt to set a central standard
according to the styleguide that we publish, to make writing Ruby at
GitHub easier.
- I left some TODOs to audit these (before we ship the new gem version!)
for whether or not they:
- a) already exist in the file in either enabled/disabled state;
- b) checking if the GitHub Ruby styleguide has opinions on them and enabling/disabling accordingly.
- This list was (as in the previous commit) generated by:
```ruby
require "yaml"
y = YAML.unsafe_load_file("show-cops.yml")
cfg = YAML.unsafe_load_file(".rubocop.yml")
on = y.keys.select { |k| y[k]["Enabled"] == true }
(on - cfg.keys).sort.each { |k| puts "#{k}:\n Enabled: true\n\n" }
```
|
Setting this to draft since there's a lot more to do here (but it's not exactly as I wrote in that last commit message). |
- This enumerates all of the currently defined rules that were implicitly disabled by `DisabledByDefault: true` and make them explicitly disabled so that there is no functional change to the RuboCop cops today e.g. if someone upgrades they won't experience a flood of newly enabled rules. - This was achieved by briefly re-enabling `DisabledByDefault`, then running `bundle exec rubocop --show-cops`, and the script included with the previous commit. - All of the TODOs from the previous commit about cross-checking newly enabled rules with the styleguide are gone, since there are no newly enabled rules, and this should be a better user experience for downstream consumers since they don't have to scramble to understand new RuboCop rules that fail in their project (over and above custom ones they have in their project which will now be enabled because we removed `DisabledByDefault` in this gem). - In the future, we'll go through the list of rules here and cross-check them with the styleguide, potentially migrating some of them to `Enabled: true`. But this changeset as it is now should be good to ship.
- The alphabetization script didn't cope very well with all the extra options for some reason.
- We've now changed tactic such that we're not enabling any new cops for downstream consumers, so let's move the "locally" disabled cops for this codebase back into `config/default.yml` so that we don't enable them for downstream consumers by accident. Something like `Lint/AssignmentInCondition` that doesn't have an autocorrect would be particularly annoying to receive violations for when you're just trying to upgrade your project's version of this gem.
- Somehow this snuck in, but RuboCop errors: ``` Error: configuration for Syntax cop found in config/default.yml. It's not possible to disable this cop. ```
…s repo" This reverts commit 53a0a37. This is because we've now disabled all the rules that would have been `DisabledByDefault`. This is so that downstream consumers don't get any nasty surprises. As a result, there should be no RuboCop linting violations in the code in this repo, so we can back out the changes to slim the diff: they're now redundant. Of course, it would be a good idea to go and make these changes at a later date, but we can do that when we enable the correct rules having matched each one up with its rationale in our styleguide.
This reverts commit e36d683. This is because we've now disabled all the rules that would have been `DisabledByDefault`. This is so that downstream consumers don't get any nasty surprises. As a result, there should be no RuboCop linting violations in the code in this repo, so we can back out the changes to slim the diff: they're now redundant. Of course, it would be a good idea to go and make these changes at a later date, but we can do that when we enable the correct rules having matched each one up with its rationale in our styleguide.
- The following message appeared when I ran `bundle exec rubocop` on this
project. Since we're setting everything to `Enabled: false` to mimic
`DisabledByDefault` for now, let's make this message go away. We almost
certainly also don't want to blanket enable all new cops, so I've not
added that configuration.
```
The following cops were added to RuboCop, but are not configured. Please set Enabled to either `true` or `false` in your `.rubocop.yml` file.
Please also note that you can opt-in to new cops by default by adding this to your config:
AllCops:
NewCops: enable
Lint/EmptyBlock: # new in 1.1
Enabled: true
Lint/EmptyClass: # new in 1.3
Enabled: true
Lint/EmptyInPattern: # new in 1.16
Enabled: true
For more information: https://docs.rubocop.org/rubocop/versioning.html
```
|
This is ready for review, please! Hopefully the commit history isn't too confusing, I feel like I've written comprehensive messages. The TL;DR is:
I've slimmed the diff down by getting rid of the fixes to the code in this repo, since we've now configured RuboCop such that all the Thanks! |
sampart
reviewed
Oct 12, 2022
|
Note that this PR also sorts the rules in |
issyl0
added a commit
to issyl0/view_components
that referenced
this pull request
Oct 21, 2022
- 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. 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`. - RuboCop does not warn users that the config inheritance has set `DisabledByDefault` somewhere up the line, leading to users mistakenly not enabling cops they may have intended to. - Other of GitHub's open source gems, like `rubocop-github`[1] and `rubocop-rails-accessibility`[2], have moved away from `DisabledByDefault`. This is the last one (at least that I can find in GitHub's main project's RuboCop gem inheritance tree). [1] - github/rubocop-github#119 [2] - github/rubocop-rails-accessibility#7
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
DisabledByDefaultconfig 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 withInclude/Exclude.This change is a follow-up to the great conversations had in config/default: Stop disabling all cops by default #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.