Rubocop: Override disabling a department

[ybiquitous]

Is your feature request related to a problem? Please describe.

Hi, I am so confused about the current behavior to disable a department.

For example, assume that there are the below two files (.rubocop.yml and test.rb):

# .rubocop.yml
Style:
  Enabled: false

Style/StringHashKeys:
  Enabled: true

# test.rb
{ "foo" => 1 }

When I looked at the .rubocop.yml above at first, I misunderstood that Style/StringHashKeys cop is enabled. 😓

However, Style/StringHashKeys cop is actually disabled in the settings above. The manual is here.

I think this behavior is very misleading. Because the settings above are equivalent to:

Style:
  Enabled: false

Style/StringHashKeys:
  Enabled: false

The settings which look different are actually the same. In the settings above, I think the behavior to "disable all Style cops except for Style/StringHashKeys" should be more natural. What do you think?

---

The reproduction example via Docker:

# Dockerfile
FROM rubylang/ruby

RUN mkdir /work
WORKDIR /work
RUN gem install rubocop:0.75 && \
    echo 'Style: { Enabled: false }' >> .rubocop.yml && \
    echo 'Style/StringHashKeys: { Enabled: true }' >> .rubocop.yml && \
    echo '{ "foo" => 1 }' > test.rb

CMD ["rubocop"]

$ docker build -t rubocop-reproduction .
...
Successfully tagged rubocop-reproduction:latest

$ docker run -it --rm rubocop-reproduction
Inspecting 1 file
.

1 file inspected, no offenses detected

In this case, I expect that a Style/StringHashKeys violation should be reported.

Describe the solution you'd like

I want a behavior that <Department>/<Cop>: { Enabled: true } overrides <Department>: { Enabled: true }. For example:

Style:
  Enabled: false  #<= Disable all Style cops

Style/StringHashKeys:
  Enabled: true   #<= Enable the cop, but other Style cops are still disabled

Describe alternatives you've considered

The solution above may be a breaking change, but I don't have good alternatives to avoid the breaking change. 🤷‍♂

Additional context

The current behavior has been added via the commit https://github.com/rubocop-hq/rubocop/commit/37943df5b62a10864c9159dd71e441d7e2304f65.

The commit has been released with the version 0.36.0:
https://github.com/rubocop-hq/rubocop/releases/tag/v0.36.0

[deivid-rodriguez]

I also find this behavior unintuitive, and it seems also inconsistent.

For example, I was using rubocop 0.63.0 (when Rails cops lived in the core repository) and I wanted to only enable the Rails/FilePath cop among Rails cops. My configuration was like this:

AllCops:
  DisabledByDefault: true

Rails:
  Enabled: true

Rails/FilePath:
  Enabled: true

After upgrading to 0.75.0, adding rubocop-rails to my Gemfile and adding it to the require: list in my configuration, I started to get many different offenses for unrelated Rails cops, and had to remove

Rails:
  Enabled: true

from my configuration to get back to the original situation. So the department level configuration seems to have a different meaning for external departments vs internal departments... or something?

[koic]

I agree with @ybiquitous. Certainly a behavior change, but it will be easier to use for intuition.

[bbatsov]

Yep, I also agree the suggested behaviour makes more sense.

[ybiquitous]

Thank you for your feedback!

I think that we need to fix the RuboCop::Config::enable_cop? method, but is it correct?
The following code snippet is my PoC:

Before

https://github.com/rubocop-hq/rubocop/blob/ef25ea2016ec15745df013ecccf94896d7183dac/lib/rubocop/config.rb#L230-L242

After

# Priority order:
# 1. Cop
# 2. Department
# 3. All
def enable_cop?(qualified_cop_name, cop_options)
  return cop_options.fetch('Enabled') if cop_options.key?('Enabled')

  cop_department, = qualified_cop_name.split('/')
  department_options = self[cop_department]
  if department_options&.key?('Enabled')
    return department_options.fetch('Enabled')
  end

  !for_all_cops['DisabledByDefault']
end

If this is not a big mistake, I will open a pull request including the code and tests.

Also, it seems that we need to fix the following document:

https://github.com/rubocop-hq/rubocop/blob/ef25ea2016ec15745df013ecccf94896d7183dac/manual/configuration.md#enabled

[buehmann]

If this is not a big mistake, I will open a pull request including the code and tests.

The problem I envision is that almost all cops have an Enabled key – inherited from default.yml. So your code would probably never get past the first line if I am not mistaken.

It needs to be clarified how this department–cop overriding interacts with inheritance from other files (be it user-defined files or default.yml).

[ybiquitous]

@buehmann Thank you for the feedback! Surely, my PoC code is wrong. 😅

It may not be enough to just fix the RuboCop::Config::enable_cop? method... 🤔

[ngouy]

What is the status of this issue?

Do you think we can leverage the en/dis-abledByDefault and bring it to the departments and cops levels?
In the default config, we could use "only" enabledByDefault: true instead of enabled: true

The enabled : true / false would have priority over dis/en-abledByDefault

So if if have

# config
Style:
  enableByDefault: false

Style/ruleA
  enabledByDefault: true

Style/ruleB
  enabledByDefault: true

by default, all Style rules are disabled but ruleA and ruleB.

# my_config which inherits from config
Style:
   disabled: true

Style/RuleA: 
  enabled: true

Here in my config, all cops Style cops but ruleA are disabled.

As a rubocop noob, I don't want to say it's a good way to solve the issue nor pretend it's a strong alternative to what we have. Just trying to help here

[ngouy]

Hi guys, me again sorry, is it possible to know the state of this issue? I am willing to work on this, I am just not sure yet which direction should be taken.

[ybiquitous]

Sorry, I'm not so familiar with RuboCop internal implementation, so I don't know how to solve this issue well... 🤷‍♂

[ybiquitous]

@jonas054 @koic This is great! I really appreciate your work! 😊 👍

[deivid-rodriguez]

Yeah, I didn't comment but I love this!! :heart_eyes:

[ngouy]

yay 🎉