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

Opt-in cop compatibility in redundant directives #10987

Merged
merged 1 commit into from Oct 28, 2022
Merged

Opt-in cop compatibility in redundant directives #10987

merged 1 commit into from Oct 28, 2022

Conversation

tdeo
Copy link
Contributor

@tdeo tdeo commented Sep 3, 2022

Add compatibility in Lint/RedundantCopDisableDirective, Lint/RedundantCopEnableDirective and Lint/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:

allowed_prefixes = %w[
  ab_
  ad_
  bc_
  ...
  zz_
]

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:

# rubocop:enable Pennylane/SortedArray
allowed_prefixes = %w[
  ...
]
# rubocop:disable Pennylane/SortedArray

But rubocop currently reports various offenses with this pattern:

sample.rb:3:18: W: [Correctable] Lint/RedundantCopEnableDirective: Unnecessary enabling of Pennylane/SortedArray.
# rubocop:enable Pennylane/SortedArray
                 ^^^^^^^^^^^^^^^^^^^^^
sample.rb:7:1: W: Lint/MissingCopEnableDirective: Re-enable Pennylane/SortedArray cop with # rubocop:enable after disabling it.
# rubocop:disable Pennylane/SortedArray
^
sample.rb:7:1: W: [Correctable] Lint/RedundantCopDisableDirective: Unnecessary disabling of Pennylane/SortedArray.
# rubocop:disable Pennylane/SortedArray

To be able to implement that, I had to inject both config as well as the cop registry into processed_source, which turns out to be pretty isolated changes in RuboCop::Runner as well as in the CopHelper 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 within CommentConfig instead of ProcessedSource, but things were getting a bit more out of hand.


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

@tdeo
Copy link
Contributor Author

tdeo commented Sep 20, 2022

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 ProcessedSource acceptable or would you prefer some other strategy (I don't have any proposal unfortunately)?

Thanks

@bbatsov
Copy link
Collaborator

bbatsov commented Sep 29, 2022

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!

@bbatsov
Copy link
Collaborator

bbatsov commented Oct 21, 2022

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.

@tdeo
Copy link
Contributor Author

tdeo commented Oct 21, 2022

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?

@bbatsov
Copy link
Collaborator

bbatsov commented Oct 23, 2022

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.

@bbatsov bbatsov merged commit 99d13e6 into rubocop:master Oct 28, 2022
@bbatsov
Copy link
Collaborator

bbatsov commented Oct 28, 2022

Thanks!

@tdeo tdeo deleted the td/opt_in_cop_compatibility branch October 28, 2022 07:41
koic added a commit to koic/rubocop-minitest that referenced this pull request Oct 29, 2022
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.
ashkulz pushed a commit to prontolabs/pronto-rubocop that referenced this pull request Nov 9, 2022
camilopayan added a commit to standardrb/standard that referenced this pull request Nov 10, 2022
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.
searls pushed a commit to standardrb/standard that referenced this pull request Dec 3, 2022
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.
leoarnold added a commit to leoarnold/templatecop that referenced this pull request Jan 7, 2023
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.
r7kamura pushed a commit to r7kamura/templatecop that referenced this pull request Jan 9, 2023
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.
@tdeo tdeo mentioned this pull request Feb 21, 2023
8 tasks
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.

None yet

2 participants