Recommend usage of require: rubocop-rails-accessibility; rename RuboCop department to "RailsAccessibility"
#7
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
…oCop department to "RailsAccessibility"
|
👋 Hello and thanks for pinging us! An accessibility first responder will review this soon.
|
bolonio
approved these changes
Oct 17, 2022
There was a problem hiding this comment.
I like the idea of the renaming and the aligning the usage with typical Rubocop libraries. Thanks for the PR 🚀
Backwards Incompatibility When a consumer of this gem upgrades, they will need to move over to the require: directive. I think RuboCop will warn about it though. If this isn't ok, we can explore some workarounds to preserve compatibility.
Once this PR is merged I will create a new version of the gem. We will need to update dotcom after that.
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.
require:directive rather thaninherit_gem:. This change also addresses the issue of Questions about default config #6 by removing theDisabledByDefaultdirective.RubocopRailsAccessibilityto simplyRailsAccessibilitywhich makes the rule naming consistent with other RuboCop plugins.RuboCop::RailsAccessibility. I noticed thatInjectwas already defined, but not loaded/used. Now it is.require:directive. I think RuboCop will warn about it though. If this isn't ok, we can explore some workarounds to preserve compatibility.rubocop-github) from Rails (rubocop-github-rails) cops and rules to disambiguate rule inheritance from custom cops rubocop-github#121