Skip to content

Commit

Permalink
[Fix rubocop#10373] Support top level departments in config
Browse files Browse the repository at this point in the history
Support for top level departments is added, but there's no
support for arbitrary levels of departments at this point.
I.e., custom cops can be under various departments, which in
turn a placed under a common top level department.

After this change it's possible to, for example, disable all
cops in the cookstyle gem by adding to user configuration:

```
Chef:
  Enabled: false
```

Note! Only the `Enabled` parameter is supported. Setting other
parameters such as `Severity` on the top level (when there are
two department levels) will silently fail. No warning is shown.
  • Loading branch information
jonas054 committed Feb 13, 2022
1 parent 1bd576e commit 284a71e
Show file tree
Hide file tree
Showing 6 changed files with 167 additions and 39 deletions.
15 changes: 8 additions & 7 deletions .rubocop_todo.yml
@@ -1,6 +1,6 @@
# This configuration was generated by
# `rubocop --auto-gen-config`
# on 2022-01-30 21:43:14 UTC using RuboCop version 1.25.0.
# on 2022-02-13 08:32:10 UTC using RuboCop version 1.25.1.
# The point is for the user to remove these configuration records
# one by one as the offenses are removed from the code base.
# Note that changes in the inspected code, or installation of new
Expand All @@ -13,9 +13,9 @@ InternalAffairs/NodeDestructuring:
# Offense count: 55
# Configuration parameters: CountComments, CountAsOne.
Metrics/ClassLength:
Max: 191
Max: 192

# Offense count: 235
# Offense count: 238
# Configuration parameters: CountComments, CountAsOne, ExcludedMethods, IgnoredMethods.
Metrics/MethodLength:
Max: 14
Expand All @@ -39,10 +39,10 @@ RSpec/AnyInstance:
RSpec/ContextWording:
Enabled: false

# Offense count: 2414
# Offense count: 2424
# Configuration parameters: CountAsOne.
RSpec/ExampleLength:
Enabled: false
Max: 126

# Offense count: 39
RSpec/ExpectOutput:
Expand All @@ -58,7 +58,7 @@ RSpec/ExpectOutput:
- 'spec/rubocop/result_cache_spec.rb'
- 'spec/rubocop/target_finder_spec.rb'

# Offense count: 404
# Offense count: 407
RSpec/MultipleExpectations:
Max: 25

Expand All @@ -72,4 +72,5 @@ RSpec/SubjectStub:

# Offense count: 12
Rake/MethodDefinitionInTask:
Enabled: false
Exclude:
- 'tasks/cut_release.rake'
6 changes: 4 additions & 2 deletions lib/rubocop/config.rb
Expand Up @@ -279,10 +279,12 @@ def enable_cop?(qualified_cop_name, cop_options)
# If the cop is explicitly enabled, the other checks can be skipped.
return true if cop_options['Enabled'] == true

department = department_of(qualified_cop_name)
cop_enabled = cop_options.fetch('Enabled') { !for_all_cops['DisabledByDefault'] }
return true if cop_enabled == 'override_department'
return false if department && department['Enabled'] == false

department_enabled = department_of(qualified_cop_name)&.dig('Enabled')
return false if department_enabled == false
return department_enabled == 'override_department' if cop_enabled == 'disabled_by_department'

cop_enabled
end
Expand Down
10 changes: 5 additions & 5 deletions lib/rubocop/config_loader_resolver.rb
Expand Up @@ -127,10 +127,9 @@ def override_department_setting_for_cops(base_hash, derived_hash)
department = Regexp.last_match(1)
next unless disabled?(derived_hash, department) || disabled?(base_hash, department)

# The `override_department` setting for the `Enabled` parameter is an
# internal setting that's not documented in the manual. It will cause a
# cop to be enabled later, when logic surrounding enabled/disabled it
# run, even though its department is disabled.
# The 'override_department' setting for the `Enabled` parameter is an internal setting
# that's not documented in the manual. It will cause a cop to be enabled later, when logic
# surrounding enabled/disabled is run, even though its department is disabled.
derived_hash[key]['Enabled'] = 'override_department' if derived_hash[key]['Enabled']
end
end
Expand All @@ -147,7 +146,8 @@ def override_enabled_for_disabled_departments(base_hash, derived_hash)
cops_to_disable.each do |cop_name|
next unless base_hash.dig(cop_name, 'Enabled') == true

derived_hash.replace(merge({ cop_name => { 'Enabled' => false } }, derived_hash))
derived_hash.replace(merge({ cop_name => { 'Enabled' => 'disabled_by_department' } },
derived_hash))
end
end

Expand Down
27 changes: 13 additions & 14 deletions lib/rubocop/config_validator.rb
Expand Up @@ -19,7 +19,7 @@ class ConfigValidator

# @api private
CONFIG_CHECK_KEYS = %w[Enabled Safe SafeAutoCorrect AutoCorrect].to_set.freeze
CONFIG_CHECK_DEPARTMENTS = %w[pending override_department].freeze
CONFIG_CHECK_DEPARTMENTS = %w[pending override_department disabled_by_department].freeze
private_constant :CONFIG_CHECK_KEYS, :CONFIG_CHECK_DEPARTMENTS

def_delegators :@config, :smart_loaded_path, :for_all_cops
Expand Down Expand Up @@ -105,13 +105,15 @@ def alert_about_unrecognized_cops(invalid_cop_names)
unknown_cops = []
invalid_cop_names.each do |name|
# There could be a custom cop with this name. If so, don't warn
next if Cop::Registry.global.contains_cop_matching?([name])
next if registry.contains_cop_matching?([name])

# Special case for inherit_mode, which is a directive that we keep in
# the configuration (even though it's not a cop), because it's easier
# to do so than to pass the value around to various methods.
next if name == 'inherit_mode'

next if departments.any? { |department| department.start_with?("#{name}/") }

message = <<~MESSAGE.rstrip
unrecognized cop or department #{name} found in #{smart_loaded_path}
#{suggestion(name)}
Expand All @@ -123,19 +125,16 @@ def alert_about_unrecognized_cops(invalid_cop_names)
end

def suggestion(name)
registry = Cop::Registry.global
departments = registry.departments.map(&:to_s)
suggestions = NameSimilarity.find_similar_names(name, departments + registry.map(&:cop_name))
if suggestions.any?
"Did you mean `#{suggestions.join('`, `')}`?"
else
# Department names can contain slashes, e.g. Chef/Correctness, but there's no support for
# the concept of higher level departments in RuboCop. It's a flat structure. So if the user
# tries to configure a "top level department", we hint that it's the bottom level
# departments that should be configured.
suggestions = departments.select { |department| department.start_with?("#{name}/") }
"#{name} is not a department. Use `#{suggestions.join('`, `')}`." if suggestions.any?
end
"Did you mean `#{suggestions.join('`, `')}`?" if suggestions.any?
end

def departments
registry.departments.map(&:to_s)
end

def registry
Cop::Registry.global
end

def validate_syntax_cop
Expand Down
133 changes: 133 additions & 0 deletions spec/rubocop/cli_spec.rb
Expand Up @@ -1281,6 +1281,139 @@ def meow_at_4am?
RESULT
end

context 'with a custom cop' do
before do
create_file('dept_config.yml', <<~YAML)
Dept:
Enabled: true
Dept/Subdept:
Enabled: true
Dept/Subdept/#{cop_name}:
Enabled: true
YAML
create_file('dept.rb', "require_relative 'lib/dept/subdept/cop'")
create_file('lib/dept/subdept/cop.rb', <<~RUBY)
module RuboCop
module Cop
module Dept
module Subdept
class #{cop_name} < Base
def on_str(node)
add_offense(node, message: "I don't like strings.")
end
end
end
end
end
end
RUBY
create_file('example.rb', <<~RUBY)
# frozen_string_literal: true
puts 'hello'
RUBY
RuboCop::ConfigLoader.default_configuration["Dept/Subdept/#{cop_name}"] = {
'Enabled' => true
}
end

around { |example| RuboCop::Cop::Registry.with_temporary_global { example.run } }

after { RuboCop::ConfigLoader.default_configuration.delete("Dept/Subdept/#{cop_name}") }

context 'with its top level department disabed' do
let(:cop_name) { 'CopA' }

it 'disables the cop' do
create_file('.rubocop.yml', <<~YAML)
inherit_from: dept_config.yml
Dept:
Enabled: false
YAML
expect(cli.run(%w[--require ./dept example.rb])).to eq(0)
end
end

context 'with its top level department disabed and its immediate department enabled' do
let(:cop_name) { 'CopB' }

it 'enables the cop and gets severity from the immediate department' do
create_file('.parent_rubocop.yml', <<~YAML)
inherit_from: dept_config.yml
Dept:
Enabled: false
Severity: error
YAML
create_file('.rubocop.yml', <<~YAML)
inherit_from: .parent_rubocop.yml
Dept/Subdept:
Enabled: true
Severity: warning
YAML
expect(cli.run(%w[--require ./dept --format simple example.rb])).to eq(1)
expect($stdout.string.strip).to eq(<<~OUTPUT.strip)
== example.rb ==
W: 3: 6: Dept/Subdept/CopB: I don't like strings.
1 file inspected, 1 offense detected
OUTPUT
end
end

context 'with its top level department disabed and its immediate department enabled and ' \
'the cop explicitly disabled' do
let(:cop_name) { 'CopE' }

it 'disables the cop' do
create_file('.rubocop.yml', <<~YAML)
Dept:
Enabled: false
Dept/Subdept:
Enabled: true
Dept/Subdept/CopE:
Enabled: false
YAML
expect(cli.run(%w[--require ./dept example.rb])).to eq(0)
end
end

context 'with its top level department enabed and its immediate department disabled' do
let(:cop_name) { 'CopC' }

it 'disables the cop' do
create_file('.parent_rubocop.yml', <<~YAML)
inherit_from: dept_config.yml
YAML
create_file('.rubocop.yml', <<~YAML)
inherit_from: .parent_rubocop.yml
Dept/Subdept:
Enabled: false
YAML
expect(cli.run(%w[--require ./dept example.rb])).to eq(0)
end
end

context 'with its top level department disabed and the cop explicitly enabled' do
let(:cop_name) { 'CopD' }

it 'disables the cop' do
create_file('.rubocop.yml', <<~YAML)
inherit_from: dept_config.yml
Dept:
Enabled: false
Dept/Subdept/CopD:
Enabled: true
YAML
expect(cli.run(%w[--require ./dept --format offenses example.rb])).to eq(1)
expect($stdout.string.strip).to eq(<<~OUTPUT.strip)
1 Dept/Subdept/CopD
--
1 Total
OUTPUT
end
end
end

it 'can use an alternative max line length from a config file' do
create_file('example_src/example1.rb', <<~RUBY)
# frozen_string_literal: true
Expand Down
15 changes: 4 additions & 11 deletions spec/rubocop/config_loader_spec.rb
Expand Up @@ -657,15 +657,6 @@ def enabled?(cop)
configuration_from_file.for_cop(cop)['Enabled']
end

if custom_dept_to_disable == 'Foo'
message = <<~'OUTPUT'.chomp
unrecognized cop or department Foo found in parent_rubocop.yml
Foo is not a department. Use `Foo/Bar`.
OUTPUT
expect { enabled?('Foo/Bar/Baz') }.to raise_error(RuboCop::ValidationError, message)
next
end

# Department disabled in parent config, cop enabled in child.
expect(enabled?('Metrics/MethodLength')).to be(true)

Expand All @@ -690,11 +681,13 @@ def enabled?(cop)
# Department disabled in grandparent, cop enabled in child config.
expect(enabled?('Lint/RaiseException')).to be(true)

# Cop enabled in grandparent, nested department disabled in parent.
# Cop enabled in grandparent, nested department (either Foo or Foo/Bar) disabled in
# parent.
expect(enabled?('Foo/Bar/Baz')).to be(false)

# Cop with similar prefix to disabled nested department.
expect(enabled?('Foo/BarBaz')).to eq(!disabled_by_default)
expect(enabled?('Foo/BarBaz')).to eq(!disabled_by_default &&
custom_dept_to_disable != 'Foo')

# Cop enabled in default config, department disabled in grandparent.
expect(enabled?('Lint/StructNewOverride')).to be(false)
Expand Down

0 comments on commit 284a71e

Please sign in to comment.