Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add new EnableNewCopsUpTo option to enable all pending cops up to a specific Rubocop version #8565

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add new EnableNewCopsUpTo option to enable all pending cops up to a specific Rubocop version #8565

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Expand Up @@ -2,6 +2,10 @@

## master (unreleased)

### New features

* [#8565](https://github.com/rubocop/rubocop/pull/8565): Add option to enable all new cops up to a given version of the plugin they came from. ([@zofrex][])

## 1.19.1 (2021-08-19)

### Bug fixes
Expand Down Expand Up @@ -5813,3 +5817,4 @@
[@markburns]: https://github.com/markburns
[@gregfletch]: https://github.com/gregfletch
[@thearjunmdas]: https://github.com/thearjunmdas
[@zofrex]: https://github.com/zofrex
2 changes: 2 additions & 0 deletions config/default.yml
View file
Open in desktop
Expand Up @@ -106,6 +106,8 @@ AllCops:
# the `--enable-pending-cops` command-line option.
# When `NewCops` is `disable`, pending cops are disabled in bulk. Can be overridden by
# the `--disable-pending-cops` command-line option.
# You can also set this to a version number per-department, e.g. `NewCops: 1.1` will
# enable all cops that were added in version 1.1 and earlier of the relevant plugin.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe "extension" is a more appropriate term.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that extensions and departments are orthogonal. An extension can add cops to a department that exists in the core RuboCop.
Making this as flexible as allowing to configure it per-extension and per department is an overkill. AllCops/NewCops for RuboCop core and optional separate per-extension is just fine.

WDYT:

AllCops:
  NewCops: 1.15

equivalent to:

AllCops:
  NewCops:
    rubocop: 1.15

For extensions:

AllCops:
  NewCops:
    rubocop: 1.15
    rubocop-rails: 2.5

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting a version number, is ambiguous.
Should a version of RuboCop core or an extension that introduced a certain cop to the existing department be used to decide if the cop is affirmed or not?

I believe some kind of warning should be issued in case of such an ambiguity, e.g.:

`Style/NewCops` is set to `1.15`. However, both `rubocop` and `rubocop-nitpicks` provide cops to the `Style` department, and their versions do not align.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a problem will be we don’t know what extension adds what cop but I agree per extension makes more sense to me than per dept.

NewCops: pending
# Enables the result cache if `true`. Can be overridden by the `--cache` command
# line option.
Expand Down
15 changes: 15 additions & 0 deletions docs/modules/ROOT/pages/versioning.adoc
View file
Open in desktop
Expand Up @@ -82,6 +82,21 @@ AllCops:

NOTE: The command-line options takes precedence over `.rubocop.yml` file.

=== Enabling/Disabling Pending Cops by Version

It is possible to enable all pending cops up to a given version of the plugin
they come from. This makes it easy to enable cops in bulk, without risking CI
breaking when future updates bring in newer cops.

[source,yaml]
----
Lint:
NewCops: 1.2
----

This would enable all cops from the Lint department that were added in Rubocop
version 1.2 or earlier, but not cops added in later versions.

=== Enabling/Disabling Individual Pending Cops

Finally, you can enable/disable individual pending cops by setting their `Enabled` configuration to either `true` or `false` in your `.rubocop.yml` file:
Expand Down
14 changes: 14 additions & 0 deletions lib/rubocop/config.rb
View file
Open in desktop
Expand Up @@ -248,10 +248,24 @@ def pending_cops
cop_metadata = self[qualified_cop_name]
next unless cop_metadata['Enabled'] == 'pending'

next if pending_cop_enabled_by_version?(qualified_cop_name, cop_metadata)

pending_cops << CopConfig.new(qualified_cop_name, cop_metadata)
end
end

def version_le(version, compare_to)
!version.nil? && !compare_to.nil? \
&& Gem::Version.correct?(version) && Gem::Version.correct?(compare_to) \
&& Gem::Version.new(version) <= Gem::Version.new(compare_to)
end

def pending_cop_enabled_by_version?(cop_name, cop_cfg)
department = department_of(cop_name)
department && !department['NewCops'].nil? && version_le(cop_cfg['VersionAdded'],
department['NewCops'])
end

private

def target_rails_version_from_bundler_lock_file
Expand Down
11 changes: 7 additions & 4 deletions lib/rubocop/cop/registry.rb
View file
Open in desktop
Expand Up @@ -156,7 +156,8 @@ def enabled(config, only = [], only_safe: false)
def enabled?(cop, config, only_safe)
cfg = config.for_cop(cop)

cop_enabled = cfg.fetch('Enabled') == true || enabled_pending_cop?(cfg, config)
cop_enabled = cfg.fetch('Enabled') == true ||
enabled_pending_cop?(cfg, config, cop.cop_name)

if only_safe
cop_enabled && cfg.fetch('Safe', true)
Expand All @@ -165,11 +166,13 @@ def enabled?(cop, config, only_safe)
end
end

def enabled_pending_cop?(cop_cfg, config)
def enabled_pending_cop?(cop_cfg, config, cop_name)
return false if @options[:disable_pending_cops]

cop_cfg.fetch('Enabled') == 'pending' &&
(@options[:enable_pending_cops] || config.enabled_new_cops?)
return false if cop_cfg.fetch('Enabled') != 'pending'

@options[:enable_pending_cops] || config.enabled_new_cops? \
|| config.pending_cop_enabled_by_version?(cop_name, cop_cfg)
end

def names
Expand Down
36 changes: 34 additions & 2 deletions spec/rubocop/cli/options_spec.rb
View file
Open in desktop
Expand Up @@ -390,7 +390,8 @@ class SomeCop < Cop

context 'when Style department is enabled' do
let(:new_cops_option) { '' }
let(:version_added) { "VersionAdded: '0.80'" }
let(:version_added) { "VersionAdded: '1.80'" }
let(:department_option) { '' }

before do
create_file('.rubocop.yml', <<~YAML)
Expand All @@ -400,6 +401,10 @@ class SomeCop < Cop
DefaultFormatter: progress
#{new_cops_option}

Style:
Enabled: true
#{department_option}

Style/SomeCop:
Description: Something
Enabled: pending
Expand All @@ -415,12 +420,39 @@ class SomeCop < Cop
remaining_range = pending_cop_warning.length..-(inspected_output.length + 1)
pending_cops = output[remaining_range]

expect(pending_cops).to include("Style/SomeCop: # new in 0.80\n Enabled: true")
expect(pending_cops).to include("Style/SomeCop: # new in 1.80\n Enabled: true")

manual_url = output[remaining_range].split("\n").last

expect(manual_url).to eq(versioning_manual_url)
end

context "when the department's NewCops option is a version > VersionAdded" do
let(:department_option) { "NewCops: '1.81'" }

it 'does not display a pending cop warning for that cop' do
expect(output).to start_with(pending_cop_warning)
expect(output).not_to include("Style/SomeCop: # new in 1.80\n Enabled: true")
end
end

context "when the department's NewCops option is a version < VersionAdded" do
let(:department_option) { "NewCops: '1.79'" }

it 'does display a pending cop warning for that cop' do
expect(output).to start_with(pending_cop_warning)
expect(output).to include("Style/SomeCop: # new in 1.80\n Enabled: true")
end
end

context "when the department's NewCops option is a version == VersionAdded" do
let(:department_option) { "NewCops: '1.80'" }

it 'does not display a pending cop warning for that cop' do
expect(output).to start_with(pending_cop_warning)
expect(output).not_to include("Style/SomeCop: # new in 1.80\n Enabled: true")
end
end
Comment on lines +430 to +455
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updating these tests for Rubocop 1.0+ highlighted how fragile they are:

  • They rely on the actual Rubocop version and will need to be changed when it passes the versions hardcoded here
  • They rely on very specific output formatting from the CLI
  • If anything updates underneath them (or a mistake is made when updating the tests), the "expect not_to include" ones have a tendency to pass, even if they shouldn't be, or aren't testing anything useful

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They rely on the actual Rubocop version and will need to be changed when it passes the versions hardcoded here

You probably could stub out the version constant for the tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree though that these tests are brittle, it’d be good if we could generate a data structure of cops that will be outputted and use that both to output and to test against, rather than testing the output itself.

At the very least, instead of expect not_to include I would test for the specific full message so that it’ll fail if something changes, but this might get overly verbose.

end

context 'when `VersionAdded` is not specified' do
Expand Down
66 changes: 66 additions & 0 deletions spec/rubocop/config_spec.rb
View file
Open in desktop
Expand Up @@ -771,6 +771,72 @@ def cop_enabled(cop_class)
end
end

context 'whether the cop is pending' do
context 'when the cop is pending' do
let(:hash) do
{
'Layout/TrailingWhitespace' => { 'Enabled' => 'pending' }
}
end

it 'is in the list of pending cops' do
expect(configuration.pending_cops).to \
include(have_attributes(name: 'Layout/TrailingWhitespace'))
end

context 'NewCops for the department is a version number' do
let(:new_cops_value) { '' }

let(:hash) do
{
'Layout' => { 'NewCops' => new_cops_value },
'Layout/TrailingWhitespace' => { 'Enabled' => 'pending', 'VersionAdded' => '0.80' }
}
end

context 'NewCops > VersionAdded' do
let(:new_cops_value) { '0.81' }

it 'is not in the list of pending cops' do
expect(configuration.pending_cops).not_to \
include(have_attributes(name: 'Layout/TrailingWhitespace'))
end
end

context 'NewCops == VersionAdded' do
let(:new_cops_value) { '0.80' }

it 'is not in the list of pending cops' do
expect(configuration.pending_cops).not_to \
include(have_attributes(name: 'Layout/TrailingWhitespace'))
end
end

context 'NewCops < VersionAdded' do
let(:new_cops_value) { '0.79' }

it 'is in the list of pending cops' do
expect(configuration.pending_cops).to \
include(have_attributes(name: 'Layout/TrailingWhitespace'))
end
end
end
end

context 'when the cop is not pending' do
let(:hash) do
{
'Layout/TrailingWhitespace' => { 'Enabled' => true }
}
end

it 'is not in the list of pending cops' do
expect(configuration.pending_cops).not_to \
include(have_attributes(name: 'Layout/TrailingWhitespace'))
end
end
end

describe '#for_department', :restore_registry do
let(:hash) do
{
Expand Down
34 changes: 34 additions & 0 deletions spec/rubocop/cop/registry_spec.rb
View file
Open in desktop
Expand Up @@ -304,6 +304,40 @@
expect(result).to include(RuboCop::Cop::Lint::BooleanSymbol)
end
end

context 'when specifying a version for NewCops in .rubocop.yml' do
let(:cops) do
[
RuboCop::Cop::Lint::EmptyBlock,
RuboCop::Cop::Lint::NoReturnInBeginEndBlocks,
RuboCop::Cop::Lint::EmptyBlock
]
end

let(:config) do
RuboCop::Config.new(
'Lint' => { 'NewCops' => '1.2' },
'Lint/EmptyBlock' => { 'Enabled' => 'pending', 'VersionAdded' => '1.1' },
'Lint/NoReturnInBeginEndBlocks' => { 'Enabled' => 'pending', 'VersionAdded' => '1.2' },
'Lint/EmptyClass' => { 'Enabled' => 'pending', 'VersionAdded' => '1.3' }
)
end

it 'includes cops added before the specified version' do
result = registry.enabled(config, [])
expect(result).to include(RuboCop::Cop::Lint::EmptyBlock)
end

it 'includes cops added in the specified version' do
result = registry.enabled(config, [])
expect(result).to include(RuboCop::Cop::Lint::NoReturnInBeginEndBlocks)
end

it 'does not include cops added after the specified version' do
result = registry.enabled(config, [])
expect(result).not_to include(RuboCop::Cop::Lint::EmptyClass)
end
end
end
end

Expand Down