From 794ec835aae456aa9860a798ac044c3308d418db Mon Sep 17 00:00:00 2001 From: Jonas Arvidsson Date: Fri, 1 May 2020 10:48:40 +0200 Subject: [PATCH] [Fix #7390] Override disabled department for enabled cops Implement the more intuitive functionality, that if a cop sets `Enabled: true` in user configuration, it will be enabled even if its department is disabled in the same configuration file, or in any inherited file. Add description of overriding Enabled in the manual. --- CHANGELOG.md | 1 + lib/rubocop/config.rb | 6 +- lib/rubocop/config_loader.rb | 3 +- lib/rubocop/config_loader_resolver.rb | 27 +++++++++ lib/rubocop/config_validator.rb | 3 +- manual/configuration.md | 17 ++++++ spec/rubocop/config_loader_spec.rb | 87 +++++++++++++++++++++++++++ 7 files changed, 141 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f6b475429a1..ad784be2c36 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ * [#7860](https://github.com/rubocop-hq/rubocop/issues/7860): Change `AllowInHeredoc` option of `Layout/TrailingWhitespace` to `true` by default. ([@koic][]) * [#7094](https://github.com/rubocop-hq/rubocop/issues/7094): Clarify alignment in `Layout/MultilineOperationIndentation`. ([@jonas054][]) +* [#7390](https://github.com/rubocop-hq/rubocop/issues/7390): **(Breaking)** Enabling a cop overrides disabling its department. ([@jonas054][]) ## 0.82.0 (2020-04-16) diff --git a/lib/rubocop/config.rb b/lib/rubocop/config.rb index 21c5309c79d..36a7615f3d1 100644 --- a/lib/rubocop/config.rb +++ b/lib/rubocop/config.rb @@ -261,9 +261,13 @@ def read_rails_version_from_bundler_lock_file def enable_cop?(qualified_cop_name, cop_options) department = department_of(qualified_cop_name) + cop_enabled = cop_options.fetch('Enabled') do + !for_all_cops['DisabledByDefault'] + end + return true if cop_enabled == 'override_department' return false if department && department['Enabled'] == false - cop_options.fetch('Enabled') { !for_all_cops['DisabledByDefault'] } + cop_enabled end def department_of(qualified_cop_name) diff --git a/lib/rubocop/config_loader.rb b/lib/rubocop/config_loader.rb index 02dc07a7878..e2aa9da1339 100644 --- a/lib/rubocop/config_loader.rb +++ b/lib/rubocop/config_loader.rb @@ -36,7 +36,7 @@ def clear_options FileFinder.root_level = nil end - def load_file(file) + def load_file(file) # rubocop:disable Metrics/AbcSize path = File.absolute_path(file.is_a?(RemoteConfig) ? file.file : file) hash = load_yaml_configuration(path) @@ -46,6 +46,7 @@ def load_file(file) add_missing_namespaces(path, hash) + resolver.override_department_setting_for_cops({}, hash) resolver.resolve_inheritance_from_gems(hash) resolver.resolve_inheritance(path, hash, file, debug?) diff --git a/lib/rubocop/config_loader_resolver.rb b/lib/rubocop/config_loader_resolver.rb index fe97c107d95..0400b97cba9 100644 --- a/lib/rubocop/config_loader_resolver.rb +++ b/lib/rubocop/config_loader_resolver.rb @@ -17,10 +17,12 @@ def resolve_requires(path, hash) end end + # rubocop:disable Metrics/MethodLength def resolve_inheritance(path, hash, file, debug) inherited_files = Array(hash['inherit_from']) base_configs(path, inherited_files, file) .reverse.each_with_index do |base_config, index| + override_department_setting_for_cops(base_config, hash) base_config.each do |k, v| next unless v.is_a?(Hash) @@ -34,6 +36,7 @@ def resolve_inheritance(path, hash, file, debug) end end end + # rubocop:enable Metrics/MethodLength def resolve_inheritance_from_gems(hash) gems = hash.delete('inherit_gem') @@ -100,8 +103,32 @@ def merge(base_hash, derived_hash, **opts) end # rubocop:enable Metrics/AbcSize + # An `Enabled: true` setting in user configuration for a cop overrides an + # `Enabled: false` setting for its department. + def override_department_setting_for_cops(base_hash, derived_hash) + derived_hash.each_key do |key| + next unless key =~ %r{(.*)/.*} + + 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. + if derived_hash[key]['Enabled'] + derived_hash[key]['Enabled'] = 'override_department' + end + end + end + private + def disabled?(hash, department) + hash[department] && hash[department]['Enabled'] == false + end + def duplicate_setting?(base_hash, derived_hash, key, inherited_file) return false if inherited_file.nil? # Not inheritance resolving merge return false if inherited_file.start_with?('..') # Legitimate override diff --git a/lib/rubocop/config_validator.rb b/lib/rubocop/config_validator.rb index 1f03c011db0..ed45529d411 100644 --- a/lib/rubocop/config_validator.rb +++ b/lib/rubocop/config_validator.rb @@ -206,7 +206,8 @@ def check_cop_config_value(hash, parent = nil) SafeAutoCorrect AutoCorrect].include?(key) && value.is_a?(String) - next if key == 'Enabled' && value == 'pending' + next if key == 'Enabled' && + %w[pending override_department].include?(value) raise ValidationError, msg_not_boolean(parent, key, value) end diff --git a/manual/configuration.md b/manual/configuration.md index 661716c36f5..148f98ad15d 100644 --- a/manual/configuration.md +++ b/manual/configuration.md @@ -446,6 +446,23 @@ Style: All cops are then enabled by default. Only cops explicitly disabled using `Enabled: false` in user configuration files are disabled. +If a department is disabled, cops in that department can still be individually +enabled, and that setting overrides the setting for its department in the same +configuration file and in any inherited file. + +```yaml +inherit_from: config_that_disables_the_metrics_department.yml + +Metrics/MethodLength: + Enabled: true + +Style: + Enabled: false + +Style/Alias: + Enabled: true +``` + ### Severity Each cop has a default severity level based on which department it belongs diff --git a/spec/rubocop/config_loader_spec.rb b/spec/rubocop/config_loader_spec.rb index 98bde0c0f97..9827febd54e 100644 --- a/spec/rubocop/config_loader_spec.rb +++ b/spec/rubocop/config_loader_spec.rb @@ -440,6 +440,93 @@ end end + context 'when a department is disabled' do + let(:file_path) { '.rubocop.yml' } + + shared_examples 'resolves enabled/disabled for all cops' do + |enabled_by_default, disabled_by_default| + it "handles EnabledByDefault: #{enabled_by_default}, " \ + "DisabledByDefault: #{disabled_by_default}" do + create_file('grandparent_rubocop.yml', <<~YAML) + Metrics/AbcSize: + Enabled: true + + Metrics/PerceivedComplexity: + Enabled: true + + Lint: + Enabled: false + YAML + create_file('parent_rubocop.yml', <<~YAML) + inherit_from: grandparent_rubocop.yml + + Metrics: + Enabled: false + + Metrics/AbcSize: + Enabled: false + YAML + create_file(file_path, <<~YAML) + inherit_from: parent_rubocop.yml + + AllCops: + EnabledByDefault: #{enabled_by_default} + DisabledByDefault: #{disabled_by_default} + + Style: + Enabled: false + + Metrics/MethodLength: + Enabled: true + + Metrics/ClassLength: + Enabled: false + + Lint/RaiseException: + Enabled: true + + Style/AndOr: + Enabled: true + YAML + + def enabled?(cop) + configuration_from_file.for_cop(cop)['Enabled'] + end + + # Department disabled in parent config, cop enabled in child. + expect(enabled?('Metrics/MethodLength')).to be(true) + + # Department disabled in parent config, cop disabled in child. + expect(enabled?('Metrics/ClassLength')).to be(false) + + # Enabled in grandparent config, disabled in parent. + expect(enabled?('Metrics/AbcSize')).to be(false) + + # Enabled in grandparent config, department disabled in parent. + expect(enabled?('Metrics/PerceivedComplexity')).to be(false) + + # Pending in default config, department disabled in grandparent. + expect(enabled?('Lint/StructNewOverride')).to be(false) + + # Department disabled in child config. + expect(enabled?('Style/Alias')).to be(false) + + # Department disabled in child config, cop enabled in child. + expect(enabled?('Style/AndOr')).to be(true) + + # Department disabled in grandparent, cop enabled in child config. + expect(enabled?('Lint/RaiseException')).to be(true) + + # Cop enabled in default config, but not mentioned in user config. + expect(enabled?('Bundler/DuplicatedGem')).to eq(!disabled_by_default) + end + end + + include_examples 'resolves enabled/disabled for all cops', false, false + include_examples 'resolves enabled/disabled for all cops', false, true + include_examples 'resolves enabled/disabled for all cops', true, false + end + context 'when a third party require defines a new gem' do before do allow(RuboCop::Cop::Cop)