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

Cannot enable rubocop-rspec derived cops on DisabledByDefault #9244

Closed
r7kamura opened this issue Dec 17, 2020 · 2 comments · Fixed by #9254
Closed

Cannot enable rubocop-rspec derived cops on DisabledByDefault #9244

r7kamura opened this issue Dec 17, 2020 · 2 comments · Fixed by #9254
Labels

Comments

@r7kamura
Copy link
Contributor

r7kamura commented Dec 17, 2020

To enable only certain cops derived from rubocop-rspec, I configured DisabledByDefault: true and tried to enable RSpec/EmptyExampleGroup like this:

# .rubocop.yml
require:
  - rubocop-rspec

AllCops:
  DisabledByDefault: true

RSpec/EmptyExampleGroup:
  Enabled: true

ref: #7923

Expected behavior

I expected RSpec/EmptyExampleGroup to be enabled like this:

$ bundle install
$ bundle exec rubocop
Inspecting 3 files
.C.

Offenses:

spec/example_spec.rb:2:3: C: RSpec/EmptyExampleGroup: Empty example group detected.
  context 'with empty example group' do
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

3 files inspected, 1 offense detected

Actual behavior

But RSpec/EmptyExampleGroup is not enabled:

$ bundle install
$ bundle exec rubocop
Inspecting 3 files
...

3 files inspected, no offenses detected

Steps to reproduce the problem

# Gemfile
source 'https://rubygems.org'

gem 'rspec'
gem 'rubocop'
gem 'rubocop-rspec'
# .rubocop.yml
require:
  - rubocop-rspec

AllCops:
  DisabledByDefault: true

RSpec/EmptyExampleGroup:
  Enabled: true
# spec/example_spec.rb
RSpec.describe 'Example' do
  context 'with empty example group' do
  end
end

RuboCop version

$ bundle exec rubocop -V
1.6.1 (using Parser 2.7.2.0, rubocop-ast 1.3.0, running on ruby 2.7.2 x86_64-linux)
  - rubocop-rspec 2.0.1
@bbatsov bbatsov added the bug label Dec 17, 2020
@r7kamura
Copy link
Contributor Author

r7kamura commented Dec 17, 2020

This problem doesn't happen on rubocop-rails. I think this is because rubocop-rspec has RSpec department in its config/default.yml, while rubocop-rails does not..

Because of this difference, RSpec department['Enabled'] returns false, while Rails department['Enabled'] returns nil .

https://github.com/rubocop-hq/rubocop/blob/f703e77ef32312c68e6fdcffcd95eb80e38200a2/lib/rubocop/config.rb#L283-L289

Maybe this false is set in this part:

https://github.com/rubocop-hq/rubocop/blob/f703e77ef32312c68e6fdcffcd95eb80e38200a2/lib/rubocop/config_loader_resolver.rb#L71-L75

override_department_setting_for_cops is called before this part, so it can't set "override_department" to RSpec/EmptyExampleGroup because RSpec dept is not yet disabled in this phase.

https://github.com/rubocop-hq/rubocop/blob/f703e77ef32312c68e6fdcffcd95eb80e38200a2/lib/rubocop/config_loader_resolver.rb#L109-L115

@dvandersluis
Copy link
Member

This was really hairy but I have a PR incoming.

dvandersluis added a commit to dvandersluis/rubocop that referenced this issue Dec 21, 2020
…nabled, ensure that it remains enabled.

With `DisabledByDefault: true` in a config, `Enabled: false` is automatically added to every key in the default configuration. In normal operation, the configuration from any config files (including via inheritence) is then merged into that default configuration, so that if a cop is explicitly enabled it overrides the default disable (in `ConfigLoaderResolver#merge_with_default`). This works even if `inherit_from` or `inherit_gem` are specified.

However, extensions inject their configuration in when they are required, and the injection code that extensions use also calls `merge_with_default`. In that case, the **extension's** configuration gets merged into rubocop's default, which then becomes the default configuration. When that configuration gets the local config merged into it with `DisabledByDefault: true`, if that extension's configuration had a department config key, it would get `Enabled: false` set on it. Then, when the specific cop is checked for if it's enabled, the department value would override the local value, and the cop would be disabled.

In order to fix this, cops with `Enabled: true` in their configuration after all the merging happens are always considered to be enabled, and the department is not checked. To facilitate this, when config files are being merged (either through inheritence or through `merge_with_default`), if a department is disabled any previously defined cops that are Enabled are flipped to be disabled (as expected, since configurations are meant to override other configurations they inherit from).
bbatsov pushed a commit that referenced this issue Dec 22, 2020
… ensure that it remains enabled.

With `DisabledByDefault: true` in a config, `Enabled: false` is automatically added to every key in the default configuration. In normal operation, the configuration from any config files (including via inheritence) is then merged into that default configuration, so that if a cop is explicitly enabled it overrides the default disable (in `ConfigLoaderResolver#merge_with_default`). This works even if `inherit_from` or `inherit_gem` are specified.

However, extensions inject their configuration in when they are required, and the injection code that extensions use also calls `merge_with_default`. In that case, the **extension's** configuration gets merged into rubocop's default, which then becomes the default configuration. When that configuration gets the local config merged into it with `DisabledByDefault: true`, if that extension's configuration had a department config key, it would get `Enabled: false` set on it. Then, when the specific cop is checked for if it's enabled, the department value would override the local value, and the cop would be disabled.

In order to fix this, cops with `Enabled: true` in their configuration after all the merging happens are always considered to be enabled, and the department is not checked. To facilitate this, when config files are being merged (either through inheritence or through `merge_with_default`), if a department is disabled any previously defined cops that are Enabled are flipped to be disabled (as expected, since configurations are meant to override other configurations they inherit from).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants