From 5fb9db3ca1fea463f75503cac0a327c57d59fb11 Mon Sep 17 00:00:00 2001 From: Phil Pirozhkov Date: Tue, 24 Dec 2019 11:11:18 +0300 Subject: [PATCH] [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 --- .rubocop_todo.yml | 4 +- CHANGELOG.md | 2 + lib/rubocop/config_loader.rb | 22 ++++- lib/rubocop/cop/generator.rb | 7 +- .../cop/generator/configuration_injector.rb | 2 +- lib/rubocop/cop/registry.rb | 9 +- .../method_call_with_args_parentheses.rb | 2 - lib/rubocop/rspec/shared_contexts.rb | 1 + manual/configuration.md | 4 +- manual/development.md | 1 - manual/index.md | 6 +- manual/versioning.md | 9 ++ spec/rubocop/config_loader_spec.rb | 83 +++++++++++++++++++ spec/rubocop/cop/generator_spec.rb | 27 +++--- spec/rubocop/cop/registry_spec.rb | 26 +++++- 15 files changed, 170 insertions(+), 35 deletions(-) create mode 100644 manual/versioning.md diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index e3190c70627..1852dea2a2f 100644 --- 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 index 21fe49366f9..a8ccc62ca9e 100644 --- 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 index 4ea337a2f2e..df106e325de 100644 --- 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 index 11a83ff95b6..8c34e2bfb11 100644 --- 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 %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 ' \ + '%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 index d00ad9d404e..3b59fb1e41d 100644 --- a/lib/rubocop/cop/generator/configuration_injector.rb +++ b/lib/rubocop/cop/generator/configuration_injector.rb @@ -10,7 +10,7 @@ class ConfigurationInjector TEMPLATE = <<~YAML %s: Description: 'TODO: Write a description of the cop.' - Enabled: true + Enabled: pending VersionAdded: '%s' YAML diff --git a/lib/rubocop/cop/registry.rb b/lib/rubocop/cop/registry.rb index b278c3dd7b4..6c82983226a 100644 --- 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 index 1a09c610a02..5157f7af606 100644 --- 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 index b322dc67d28..edd36872e1a 100644 --- 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 index 72678b40d1c..8656da5a57a 100644 --- 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 index 48ae70f7cef..001d1170e5a 100644 --- 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 index 8d30312b4f8..6b6eba32181 100644 --- a/manual/index.md +++ b/manual/index.md @@ -7,8 +7,7 @@ **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. @@ -16,6 +15,9 @@ 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 new file mode 100644 index 00000000000..c51d34426cb --- /dev/null +++ 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 index be5ff7dabc3..d2390cc7425 100644 --- 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 index 8040c6568bc..618ea5be1f1 100644 --- 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 index 2cab21b050f..fc23b5123af 100644 --- 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