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

rubocop/config/default: Unset DisabledByDefault #1531

Merged
merged 2 commits into from
Oct 21, 2022

Conversation

issyl0
Copy link
Contributor

@issyl0 issyl0 commented Oct 21, 2022

Description

  • 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

Integration

Does this change require any updates to code in production?

Nope.

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Added/updated previews

- 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
@issyl0 issyl0 requested review from a team and BlakeWilliams October 21, 2022 14:54
@changeset-bot
Copy link

changeset-bot bot commented Oct 21, 2022

🦋 Changeset detected

Latest commit: be6fe65

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/view-components Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Member

@jonrohan jonrohan left a comment

Choose a reason for hiding this comment

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

Thanks for the clear explanation. This should be in the next release 105, probably out next week

@jonrohan jonrohan enabled auto-merge (squash) October 21, 2022 16:05
@jonrohan jonrohan merged commit 58436f7 into primer:main Oct 21, 2022
@primer-css primer-css mentioned this pull request Oct 21, 2022
@issyl0 issyl0 deleted the rubocop-unset-disabled-by-default branch October 21, 2022 16:29
@camertron
Copy link
Contributor

camertron commented Oct 21, 2022

Hey @issyl0, unfortunately we need to revert this PR because it's causing an issue with our CI builds at the moment.

The problem is erblint only looks at snippets of Ruby code, eg <%= render(MyComponent.new) %>. In this example, applying regular 'ol Rubocop rules will say the line begins with indentation characters because it sees [space]render(MyComponent.new)[space]. That's why our config originally disabled all cops and enabled only a select few. We should probably have a separate erblint and Rubocop configs.

@camertron
Copy link
Contributor

Ok, it looks like disabling cops can be done directly inside .erb-lint.yml 🎉 Here's the PR: #1535

@issyl0
Copy link
Contributor Author

issyl0 commented Oct 24, 2022

Thanks and sorry for the friction here @camertron, I didn't know you used DisabledByDefault in this way as a considered choice. Doing it in this project's erb-lint.yml for your tests makes sense to me, so it doesn't pollute the gem config that is shipped to users.

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.

4 participants