-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Opt-in cop compatibility in redundant directives #10987
Opt-in cop compatibility in redundant directives #10987
Conversation
Hello dear rubocop team, I'd love to get some feedback on this - does this functionality make sense to be added into rubocop? Is the modification to Thanks |
I've been AFK much of September and I'll need some time to go over all the PRs in our backlog. Sorry about the delay! |
So, if I understand this feature correctly you want to be using the directives to temporary enable a cop, right? If so - I'm fine with the proposal and I'll only request that this functionality be documented accordingly. |
That's exactly the functionality I have in mind, I was calling them "opt-in" cops trying to mean they're disabled by default, but as you're saying "temporarily enabled cops" might be more explicit. Where would you like to see the documentation? Would a paragraph "Temporarily enabling cops in source code" right after "Disabling cops within source code" here work? |
Yep, that would be the right place IMO. |
Thanks! |
Follow up rubocop/rubocop#10987. ```console % bundle exec rake (snip) NoMethodError: undefined method `disabled' for nil:NilClass registry.disabled(config).each do |cop| ^^^^^^^^^ /Users/koic/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/bundler/gems/rubocop-479e588e16cd/lib/rubocop/comment_config.rb:94:in `inject_disabled_cops_directives' /Users/koic/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/bundler/gems/rubocop-479e588e16cd/lib/rubocop/comment_config.rb:78:in `analyze' /Users/koic/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/bundler/gems/rubocop-479e588e16cd/lib/rubocop/comment_config.rb:45:in `cop_disabled_line_ranges' ``` It supports multiple RuboCop core versions for compatibility with rubocop#156.
This was added to accomodate the changes in [this pull request](rubocop/rubocop#10987) It does seem as though these changes should be reflected in the ProcessedSource module of rubocop-AST or in the cop model in rubocop, and shouldn't need this change to our test code.
This was added to accomodate the changes in [this pull request](rubocop/rubocop#10987) It does seem as though these changes should be reflected in the ProcessedSource module of rubocop-AST or in the cop model in rubocop, and shouldn't need this change to our test code.
Due to rubocop/rubocop#10987 we need to set `ProcessedSource#config` and `ProcessedSource#registry`. Not doing so caused `Lint/MissingCopEnableDirective` and `Lint/RedundantCopDisableDirective` cops to crash.
Due to rubocop/rubocop#10987 we need to set `ProcessedSource#config` and `ProcessedSource#registry`. Not doing so caused `Lint/MissingCopEnableDirective` and `Lint/RedundantCopDisableDirective` cops to crash.
Add compatibility in
Lint/RedundantCopDisableDirective
,Lint/RedundantCopEnableDirective
andLint/MissingCopEnableDirective
for opt-in cops.I mean by "opt-in" having a cop which is globally disabled in the configuration, but that we want to enable only on specific parts of a file.
My use-case specifically is the following: we sometimes have large-ish arrays maintaining list of allowed values for an attribute, let's imagine:
I wrote a custom cop that checks that this array remains alphabetically sorted for human readibility. Obviously, I don't want to enable it globally in the codebase as array ordering is important in many use-cases, but would like to enable it specifically on that part with:
But rubocop currently reports various offenses with this pattern:
To be able to implement that, I had to inject both
config
as well as the cop registry intoprocessed_source
, which turns out to be pretty isolated changes inRuboCop::Runner
as well as in theCopHelper
test module to mimic the behaviour.I also made a small api change to
RuboCop::Registry#enabled
as 2 arguments were already available from instance variables, please let me know if that isn't an acceptable change.I didn't squash commits yet to be able to discuss implementation strategy. I originally (until bf4e4b7) tried to only inject
config
withinCommentConfig
instead ofProcessedSource
, but things were getting a bit more out of hand.Before submitting the PR make sure the following are checked:
[Fix #issue-number]
(if the related issue exists).master
(if not - rebase it).bundle exec rake default
. It executes all tests and runs RuboCop on its own code.{change_type}_{change_description}.md
if the new code introduces user-observable changes. See changelog entry format for details.