[Fix #5979] Introduce cops with special status (#7567)

Previously, new cops were introduced as either enabled or disabled. The ones enabled were bothering users with new offences, while disabled cops were often left out and remained under their radars, while still being useful. By introducing this special status, users have to decide how to handle new cops, by explicitly enabling or disabling them. Cops are to be introduced with pending status between major releases of RuboCop and its extensions, and they eventually become enabled or disabled on major releases. Co-authored-by: Phil Pirozhkov <hello@fili.pp.ru>
rubocop · Dec 24, 2019 · 5fb9db3 · 5fb9db3
1 parent 46e412e
commit 5fb9db3
Show file tree

Hide file tree

Showing 15 changed files with 170 additions and 35 deletions.
diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml
@@ -10,10 +10,10 @@
 InternalAffairs/NodeDestructuring:
   Enabled: false
 
-# Offense count: 49
+# Offense count: 50
 # Configuration parameters: CountComments.
 Metrics/ClassLength:
-  Max: 183
+  Max: 197
 
 # Offense count: 209
 # Configuration parameters: CountComments, ExcludedMethods.

diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -12,6 +12,7 @@
 
 * [#7528](https://github.com/rubocop-hq/rubocop/pull/7528): Add new `Lint/NonDeterministicRequireOrder` cop. ([@mangara][])
 * [#7559](https://github.com/rubocop-hq/rubocop/pull/7559): Add `EnforcedStyleForExponentOperator` parameter to `Layout/SpaceAroundOperators` cop. ([@khiav223577][])
+* [#7567](https://github.com/rubocop-hq/rubocop/pull/7567): Introduce new `pending` status for new cops. ([@Darhazer][], [@pirj][])
 
 ### Bug fixes
 
@@ -4297,3 +4298,4 @@
 [@movermeyer]: https://github.com/movermeyer
 [@jethroo]: https://github.com/jethroo
 [@mangara]: https://github.com/mangara
+[@pirj]: https://github.com/pirj
diff --git a/lib/rubocop/config_loader.rb b/lib/rubocop/config_loader.rb
@@ -91,7 +91,9 @@ def configuration_from_file(config_file)
         else
           add_excludes_from_files(config, config_file)
         end
-        merge_with_default(config, config_file)
+        merge_with_default(config, config_file).tap do |merged_config|
+          warn_on_pending_cops(merged_config)
+        end
       end
 
       def add_excludes_from_files(config, config_file)
@@ -114,6 +116,22 @@ def default_configuration
                                    end
       end
 
+      def warn_on_pending_cops(config)
+        pending_cops = config.keys.select do |key|
+          config[key]['Enabled'] == 'pending'
+        end
+
+        return if pending_cops.none?
+
+        warn Rainbow('The following cops were added to RuboCop, but are not ' \
+                     'configured. Please set Enabled to either `true` or ' \
+                     '`false` in your `.rubocop.yml` file:').yellow
+
+        pending_cops.each do |cop|
+          warn Rainbow(" - #{cop}").yellow
+        end
+      end
+
       # Merges the given configuration with the default one. If
       # AllCops:DisabledByDefault is true, it changes the Enabled params so
       # that only cops from user configuration are enabled.
@@ -231,6 +249,8 @@ def check_cop_config_value(hash, parent = nil)
                          SafeAutoCorrect
                          AutoCorrect].include?(key) && value.is_a?(String)
 
+          next if key == 'Enabled' && value == 'pending'
+
           abort(
             "Property #{Rainbow(key).yellow} of cop #{Rainbow(parent).yellow}" \
             " is supposed to be a boolean and #{Rainbow(value).yellow} is not."

diff --git a/lib/rubocop/cop/generator.rb b/lib/rubocop/cop/generator.rb
@@ -104,10 +104,9 @@ def on_send(node)
         end
       SPEC
 
-      CONFIGURATION_ADDED_MESSAGE = <<~MESSAGE
-        [modify] A configuration for the cop is added into %<configuration_file_path>s.
-                 If you want to disable the cop by default, set `Enabled` option to false.
-      MESSAGE
+      CONFIGURATION_ADDED_MESSAGE =
+        '[modify] A configuration for the cop is added into ' \
+          '%<configuration_file_path>s.'
 
       def initialize(name, github_user, output: $stdout)
         @badge = Badge.parse(name)

diff --git a/lib/rubocop/cop/generator/configuration_injector.rb b/lib/rubocop/cop/generator/configuration_injector.rb
@@ -10,7 +10,7 @@ class ConfigurationInjector
         TEMPLATE = <<~YAML
           %<badge>s:
             Description: 'TODO: Write a description of the cop.'
-            Enabled: true
+            Enabled: pending
             VersionAdded: '%<version_added>s'
         YAML
 

diff --git a/lib/rubocop/cop/registry.rb b/lib/rubocop/cop/registry.rb
@@ -146,10 +146,15 @@ def enabled(config, only, only_safe = false)
 
       def enabled?(cop, config, only_safe)
         cfg = config.for_cop(cop)
+
+        # cfg['Enabled'] might be a string `pending`, which is considered
+        # disabled
+        cop_enabled = cfg.fetch('Enabled') == true
+
         if only_safe
-          cfg.fetch('Enabled') && cfg.fetch('Safe', true)
+          cop_enabled && cfg.fetch('Safe', true)
         else
-          cfg.fetch('Enabled')
+          cop_enabled
         end
       end
 

diff --git a/lib/rubocop/cop/style/method_call_with_args_parentheses.rb b/lib/rubocop/cop/style/method_call_with_args_parentheses.rb
@@ -1,6 +1,5 @@
 # frozen_string_literal: true
 
-# rubocop:disable Metrics/ClassLength
 module RuboCop
   module Cop
     module Style
@@ -385,4 +384,3 @@ def assigned_before?(node, target)
     end
   end
 end
-# rubocop:enable Metrics/ClassLength
diff --git a/lib/rubocop/rspec/shared_contexts.rb b/lib/rubocop/rspec/shared_contexts.rb
@@ -52,6 +52,7 @@
       cop_name = described_class.cop_name
       hash[cop_name] = RuboCop::ConfigLoader
                        .default_configuration[cop_name]
+                       .merge('Enabled' => true) # in case it is 'pending'
                        .merge(cop_config)
     end
 

diff --git a/manual/configuration.md b/manual/configuration.md
@@ -406,7 +406,9 @@ Layout/LineLength:
   Enabled: false
 ```
 
-Most cops are enabled by default. Some cops, configured the above `Enabled: false`
+Most cops are enabled by default. Cops, introduced or significantly updated
+between major versions, are in a special pending status (read more in
+["Versioning"](versioning.md)). Some cops, configured the above `Enabled: false`
 in [config/default.yml](https://github.com/rubocop-hq/rubocop/blob/master/config/default.yml),
 are disabled by default. The cop enabling process can be altered by
 setting `DisabledByDefault` or `EnabledByDefault` (but not both) to `true`.

diff --git a/manual/development.md b/manual/development.md
@@ -10,7 +10,6 @@ Files created:
 File modified:
   - `require_relative 'rubocop/cop/department/name'` added into lib/rubocop.rb
   - A configuration for the cop is added into config/default.yml
-    - If you want to disable the cop by default, set `Enabled` option to false.
 
 Do 3 steps:
   1. Add an entry to the "New features" section in CHANGELOG.md,

diff --git a/manual/index.md b/manual/index.md
@@ -7,15 +7,17 @@
 
 **RuboCop** is a Ruby static code analyzer (a.k.a. linter) and code
 formatter. Out of the box it will enforce many of the guidelines
-outlined in the community [Ruby Style
-Guide](https://rubystyle.guide).
+outlined in the community [Ruby Style Guide](https://rubystyle.guide).
 
 Apart from reporting problems in your code, RuboCop can also
 automatically fix some of the problems for you.
 
 See ["Basic Usage"](basic_usage.md) to get yourself familiar with RuboCop's
 capabilities.
 
+See ["Versioning"](versioning.md) for information about RuboCop versioning,
+updates, and introduction of new cops.
+
 RuboCop is an extremely flexible tool and most aspects of its behavior
 can be tweaked via various [configuration
 options](https://github.com/rubocop-hq/rubocop/blob/master/config/default.yml).
diff --git a/manual/versioning.md b/manual/versioning.md
@@ -0,0 +1,9 @@
+## Versioning
+
+RuboCop is stable between major versions, both in terms of API and cops.
+
+New cops introduced between major versions are set to a special pending
+status and are not enabled by default. A warning is emitted if such cops
+and are not explicitly enabled or disabled in the user configuration.
+
+On major version updates, pending cops are enabled in bulk.
diff --git a/spec/rubocop/config_loader_spec.rb b/spec/rubocop/config_loader_spec.rb
@@ -904,6 +904,89 @@ def cop_enabled?(cop_class)
         end
       end
     end
+
+    context 'when a new cop is introduced' do
+      def cop_enabled?(cop_class)
+        configuration_from_file.for_cop(cop_class).fetch('Enabled')
+      end
+
+      let(:file_path) { '.rubocop.yml' }
+      let(:cop_class) { RuboCop::Cop::Metrics::MethodLength }
+
+      before do
+        stub_const('RuboCop::ConfigLoader::RUBOCOP_HOME', 'rubocop')
+        stub_const('RuboCop::ConfigLoader::DEFAULT_FILE',
+                   File.join('rubocop', 'config', 'default.yml'))
+        create_file('rubocop/config/default.yml',
+                    <<~YAML)
+                      AllCops:
+                        AnythingGoes: banana
+                      Metrics/MethodLength:
+                        Enabled: pending
+                    YAML
+        create_file(file_path, config)
+      end
+
+      context 'when not configured explicitly' do
+        let(:config) { '' }
+
+        it 'is disabled' do
+          expect(cop_enabled?(cop_class)).to eq 'pending'
+        end
+      end
+
+      context 'when enabled explicitly in config' do
+        let(:config) do
+          <<~YAML
+            Metrics/MethodLength:
+              Enabled: true
+          YAML
+        end
+
+        it 'is enabled' do
+          expect(cop_enabled?(cop_class)).to be true
+        end
+      end
+
+      context 'when disabled explicitly in config' do
+        let(:config) do
+          <<~YAML
+            Metrics/MethodLength:
+              Enabled: false
+          YAML
+        end
+
+        it 'is disabled' do
+          expect(cop_enabled?(cop_class)).to be false
+        end
+      end
+
+      context 'when DisabledByDefault is true' do
+        let(:config) do
+          <<~YAML
+            AllCops:
+              DisabledByDefault: true
+          YAML
+        end
+
+        it 'is disabled' do
+          expect(cop_enabled?(cop_class)).to be false
+        end
+      end
+
+      context 'when EnabledByDefault is true' do
+        let(:config) do
+          <<~YAML
+            AllCops:
+              EnabledByDefault: true
+          YAML
+        end
+
+        it 'is enabled' do
+          expect(cop_enabled?(cop_class)).to be true
+        end
+      end
+    end
   end
 
   describe '.load_file', :isolated_environment do

diff --git a/spec/rubocop/cop/generator_spec.rb b/spec/rubocop/cop/generator_spec.rb
@@ -205,7 +205,7 @@ def on_send(node)
 
           Style/FakeCop:
             Description: 'TODO: Write a description of the cop.'
-            Enabled: true
+            Enabled: pending
             VersionAdded: '0.59'
 
           Style/Lambda:
@@ -217,10 +217,9 @@ def on_send(node)
 
         generator.inject_config(config_file_path: path)
 
-        expect(stdout.string).to eq(<<~MESSAGE)
-          [modify] A configuration for the cop is added into #{path}.
-                   If you want to disable the cop by default, set `Enabled` option to false.
-        MESSAGE
+        expect(stdout.string)
+          .to eq('[modify] A configuration for the cop is added into ' \
+                 "#{path}.\n")
       end
     end
 
@@ -231,7 +230,7 @@ def on_send(node)
         expect(File).to receive(:write).with(path, <<~YAML)
           Style/Aaa:
             Description: 'TODO: Write a description of the cop.'
-            Enabled: true
+            Enabled: pending
             VersionAdded: '0.59'
 
           Style/Alias:
@@ -246,10 +245,9 @@ def on_send(node)
 
         generator.inject_config(config_file_path: path)
 
-        expect(stdout.string).to eq(<<~MESSAGE)
-          [modify] A configuration for the cop is added into #{path}.
-                   If you want to disable the cop by default, set `Enabled` option to false.
-        MESSAGE
+        expect(stdout.string)
+          .to eq('[modify] A configuration for the cop is added into ' \
+                 "#{path}.\n")
       end
     end
 
@@ -269,16 +267,15 @@ def on_send(node)
 
           Style/Zzz:
             Description: 'TODO: Write a description of the cop.'
-            Enabled: true
+            Enabled: pending
             VersionAdded: '0.59'
         YAML
 
         generator.inject_config(config_file_path: path)
 
-        expect(stdout.string).to eq(<<~MESSAGE)
-          [modify] A configuration for the cop is added into #{path}.
-                   If you want to disable the cop by default, set `Enabled` option to false.
-        MESSAGE
+        expect(stdout.string)
+          .to eq('[modify] A configuration for the cop is added into ' \
+                 "#{path}.\n")
       end
     end
   end

diff --git a/spec/rubocop/cop/registry_spec.rb b/spec/rubocop/cop/registry_spec.rb
@@ -62,7 +62,7 @@ class Foo < Cop
       .to eq(described_class.new(cops.drop(2)))
   end
 
-  context '#contains_cop_matching?' do
+  describe '#contains_cop_matching?' do
     it 'can find cops matching a given name' do
       result = registry.contains_cop_matching?(
         ['Test/FirstArrayElementIndentation']
@@ -75,7 +75,7 @@ class Foo < Cop
     end
   end
 
-  context '#qualified_cop_name' do
+  describe '#qualified_cop_name' do
     let(:origin) { '/app/.rubocop.yml' }
 
     it 'gives back already properly qualified names' do
@@ -155,7 +155,7 @@ class Foo < Cop
     )
   end
 
-  context '#cops' do
+  describe '#cops' do
     it 'exposes a list of cops' do
       expect(registry.cops).to eql(cops)
     end
@@ -165,7 +165,7 @@ class Foo < Cop
     expect(registry.length).to be(6)
   end
 
-  context '#enabled' do
+  describe '#enabled' do
     let(:config) do
       RuboCop::Config.new(
         'Test/FirstArrayElementIndentation' => { 'Enabled' => false },
@@ -186,6 +186,24 @@ class Foo < Cop
       enabled_cops = registry.enabled(config, [], true)
       expect(enabled_cops).not_to include(RuboCop::Cop::RSpec::Foo)
     end
+
+    context 'when new cops are introduced' do
+      let(:config) do
+        RuboCop::Config.new(
+          'Lint/BooleanSymbol' => { 'Enabled' => 'pending' }
+        )
+      end
+
+      it 'does not include them' do
+        result = registry.enabled(config, [])
+        expect(result).not_to include(RuboCop::Cop::Lint::BooleanSymbol)
+      end
+
+      it 'overrides config if :only includes the cop' do
+        result = registry.enabled(config, ['Lint/BooleanSymbol'])
+        expect(result).to eql(cops)
+      end
+    end
   end
 
   it 'exposes a list of cop names' do