From 284a71e9a574f4ffcea1d626bb166b1c77a5d4dc Mon Sep 17 00:00:00 2001 From: Jonas Arvidsson Date: Sun, 30 Jan 2022 14:41:42 +0100 Subject: [PATCH] [Fix #10373] Support top level departments in config 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. --- .rubocop_todo.yml | 15 +-- lib/rubocop/config.rb | 6 +- lib/rubocop/config_loader_resolver.rb | 10 +- lib/rubocop/config_validator.rb | 27 +++--- spec/rubocop/cli_spec.rb | 133 ++++++++++++++++++++++++++ spec/rubocop/config_loader_spec.rb | 15 +-- 6 files changed, 167 insertions(+), 39 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index e48f76c5513..1d3e29c49cb 100644 --- a/.rubocop_todo.yml +++ b/.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 @@ -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 @@ -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: @@ -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 @@ -72,4 +72,5 @@ RSpec/SubjectStub: # Offense count: 12 Rake/MethodDefinitionInTask: - Enabled: false + Exclude: + - 'tasks/cut_release.rake' diff --git a/lib/rubocop/config.rb b/lib/rubocop/config.rb index 9bdcad1ec95..03d9b756598 100644 --- a/lib/rubocop/config.rb +++ b/lib/rubocop/config.rb @@ -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 diff --git a/lib/rubocop/config_loader_resolver.rb b/lib/rubocop/config_loader_resolver.rb index 72d979634c5..27db7c1d41a 100644 --- a/lib/rubocop/config_loader_resolver.rb +++ b/lib/rubocop/config_loader_resolver.rb @@ -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 @@ -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 diff --git a/lib/rubocop/config_validator.rb b/lib/rubocop/config_validator.rb index 8d4868350bc..b25fbb3aaf6 100644 --- a/lib/rubocop/config_validator.rb +++ b/lib/rubocop/config_validator.rb @@ -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 @@ -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)} @@ -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 diff --git a/spec/rubocop/cli_spec.rb b/spec/rubocop/cli_spec.rb index 7db28135fd8..1b93335d4db 100644 --- a/spec/rubocop/cli_spec.rb +++ b/spec/rubocop/cli_spec.rb @@ -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 diff --git a/spec/rubocop/config_loader_spec.rb b/spec/rubocop/config_loader_spec.rb index 8eb77cc8ea4..2eae8c6516a 100644 --- a/spec/rubocop/config_loader_spec.rb +++ b/spec/rubocop/config_loader_spec.rb @@ -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) @@ -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)