Merge pull request #7923 from jonas054/7390_cop_enabled_overrides_dep…

…artment

[Fix #7390] Override disabled department for enabled cops

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)
 

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)

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?)
 

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

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

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

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)