From fcea71adce17d9d6913125db333970f0b01b6bd6 Mon Sep 17 00:00:00 2001 From: Dmytro Savochkin Date: Fri, 16 Oct 2020 12:03:49 +0300 Subject: [PATCH] [Fix #8044] Change cop department name calculation --- .rubocop.yml | 35 ++++++++++++++++++ CHANGELOG.md | 1 + lib/rubocop/cop/badge.rb | 33 +++++------------ spec/rubocop/cop/badge_spec.rb | 57 ++++++++++++++++++++++-------- spec/rubocop/cop/generator_spec.rb | 3 +- spec/rubocop/cop/registry_spec.rb | 15 ++++++++ 6 files changed, 105 insertions(+), 39 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index 852b2f71932..060634357cc 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -135,3 +135,38 @@ Performance/CollectionLiteralInLoop: Exclude: - 'Rakefile' - 'spec/**/*.rb' + +# TODO: remove this after rubocop-rspec updated to support deep departments names +RSpec/Capybara/CurrentPathExpectation: + Description: Checks that no expectations are set on Capybara's `current_path`. + Enabled: true + +# TODO: remove this after rubocop-rspec updated to support deep departments names +RSpec/Capybara/FeatureMethods: + Description: Checks for consistent method usage in feature specs. + Enabled: true + EnabledMethods: [] + +# TODO: remove this after rubocop-rspec updated to support deep departments names +RSpec/Capybara/VisibilityMatcher: + Description: Checks for boolean visibility in capybara finders. + Enabled: true + +# TODO: remove this after rubocop-rspec updated to support deep departments names +RSpec/FactoryBot/AttributeDefinedStatically: + Description: Always declare attribute values as blocks. + Enabled: true + +# TODO: remove this after rubocop-rspec updated to support deep departments names +RSpec/FactoryBot/CreateList: + Description: Checks for create_list usage. + Enabled: true + EnforcedStyle: create_list + SupportedStyles: + - create_list + - n_times + +# TODO: remove this after rubocop-rspec updated to support deep departments names +RSpec/FactoryBot/FactoryClassName: + Description: Use string value when setting the class attribute explicitly. + Enabled: true diff --git a/CHANGELOG.md b/CHANGELOG.md index 1784298a0f2..07666b374d7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ * [#8882](https://github.com/rubocop-hq/rubocop/pull/8882): **(Potentially breaking)** RuboCop assumes that Cop classes do not define new `on_` methods at runtime (e.g. via `extend` in `initialize`). ([@marcandre][]) * [#7966](https://github.com/rubocop-hq/rubocop/issues/7966): **(Breaking)** Enable all pending cops for RuboCop 1.0. ([@koic][]) +* [#8490](https://github.com/rubocop-hq/rubocop/pull/8490): **(Breaking)** Change logic for cop department name computation. Cops inside deep namespaces (5 or more levels deep) now belong to departments with names that are calculated by joining module names starting from the third one with slashes as separators. For example, cop `Rubocop::Cop::Foo::Bar::Baz` now belongs to `Foo/Bar` department (previously it was `Bar`). ([@dsavochkin][]) ## 0.93.1 (2020-10-12) diff --git a/lib/rubocop/cop/badge.rb b/lib/rubocop/cop/badge.rb index be1710879c4..9bba9656474 100644 --- a/lib/rubocop/cop/badge.rb +++ b/lib/rubocop/cop/badge.rb @@ -10,37 +10,22 @@ module Cop # allow for badge references in source files that omit the department for # RuboCop to infer. class Badge - # Error raised when a badge parse fails. - class InvalidBadge < Error - MSG = 'Invalid badge %p. ' \ - 'Expected `Department/CopName` or `CopName`.' - - def initialize(token) - super(format(MSG, badge: token)) - end - end - attr_reader :department, :cop_name def self.for(class_name) - new(*class_name.split('::').last(2)) + parts = class_name.split('::') + name_deep_enough = parts.length >= 4 + new(name_deep_enough ? parts[2..-1] : parts.last(2)) end def self.parse(identifier) - parts = identifier.split('/', 2) - - raise InvalidBadge, identifier if parts.size > 2 - - if parts.one? - new(nil, *parts) - else - new(*parts) - end + new(identifier.split('/')) end - def initialize(department, cop_name) - @department = department.to_sym if department - @cop_name = cop_name + def initialize(class_name_parts) + department_parts = class_name_parts[0...-1] + @department = (department_parts.join('/').to_sym unless department_parts.empty?) + @cop_name = class_name_parts.last end def ==(other) @@ -66,7 +51,7 @@ def qualified? end def with_department(department) - self.class.new(department, cop_name) + self.class.new([department.to_s.split('/'), cop_name].flatten) end end end diff --git a/spec/rubocop/cop/badge_spec.rb b/spec/rubocop/cop/badge_spec.rb index c2338ef6e76..9ee31f2179d 100644 --- a/spec/rubocop/cop/badge_spec.rb +++ b/spec/rubocop/cop/badge_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true RSpec.describe RuboCop::Cop::Badge do - subject(:badge) { described_class.new('Test', 'ModuleMustBeAClassCop') } + subject(:badge) { described_class.new(%w[Test ModuleMustBeAClassCop]) } it 'exposes department name' do expect(badge.department).to be(:Test) @@ -11,34 +11,59 @@ expect(badge.cop_name).to eql('ModuleMustBeAClassCop') end - describe '.parse' do - it 'parses Department/CopName syntax' do - expect(described_class.parse('Foo/Bar')) - .to eq(described_class.new('Foo', 'Bar')) + describe '.new' do + shared_examples 'assignment of department and name' do |class_name_parts, department, name| + it 'assigns department' do + expect(described_class.new(class_name_parts).department).to eq(department) + end + + it 'assigns name' do + expect(described_class.new(class_name_parts).cop_name).to eq(name) + end end - it 'parses unqualified badge references' do - expect(described_class.parse('Bar')) - .to eql(described_class.new(nil, 'Bar')) + include_examples 'assignment of department and name', %w[Foo], nil, 'Foo' + include_examples 'assignment of department and name', %w[Foo Bar], :Foo, 'Bar' + include_examples 'assignment of department and name', %w[Foo Bar Baz], :'Foo/Bar', 'Baz' + include_examples 'assignment of department and name', %w[Foo Bar Baz Qux], :'Foo/Bar/Baz', 'Qux' + end + + describe '.parse' do + shared_examples 'cop identifier parsing' do |identifier, class_name_parts| + it 'parses identifier' do + expect(described_class.parse(identifier)).to eq(described_class.new(class_name_parts)) + end end + + include_examples 'cop identifier parsing', 'Bar', %w[Bar] + include_examples 'cop identifier parsing', 'Foo/Bar', %w[Foo Bar] + include_examples 'cop identifier parsing', 'Foo/Bar/Baz', %w[Foo Bar Baz] + include_examples 'cop identifier parsing', 'Foo/Bar/Baz/Qux', %w[Foo Bar Baz Qux] end describe '.for' do - it 'parses cop class name' do - expect(described_class.for('RuboCop::Cop::Foo::Bar')) - .to eq(described_class.new('Foo', 'Bar')) + shared_examples 'cop class name parsing' do |class_name, class_name_parts| + it 'parses cop class name' do + expect(described_class.for(class_name)).to eq(described_class.new(class_name_parts)) + end end + + include_examples 'cop class name parsing', 'Foo', %w[Foo] + include_examples 'cop class name parsing', 'Foo::Bar', %w[Foo Bar] + include_examples 'cop class name parsing', 'RuboCop::Cop::Foo', %w[Cop Foo] + include_examples 'cop class name parsing', 'RuboCop::Cop::Foo::Bar', %w[Foo Bar] + include_examples 'cop class name parsing', 'RuboCop::Cop::Foo::Bar::Baz', %w[Foo Bar Baz] end it 'compares by value' do - badge1 = described_class.new('Foo', 'Bar') - badge2 = described_class.new('Foo', 'Bar') + badge1 = described_class.new(%w[Foo Bar]) + badge2 = described_class.new(%w[Foo Bar]) expect(Set.new([badge1, badge2]).one?).to be(true) end it 'can be converted to a string with the Department/CopName format' do - expect(described_class.new('Foo', 'Bar').to_s).to eql('Foo/Bar') + expect(described_class.new(%w[Foo Bar]).to_s).to eql('Foo/Bar') end describe '#qualified?' do @@ -49,5 +74,9 @@ it 'says `Department/CopName` is qualified' do expect(described_class.parse('Department/Bar').qualified?).to be(true) end + + it 'says `Deep/Department/CopName` is qualified' do + expect(described_class.parse('Deep/Department/Bar').qualified?).to be(true) + end end end diff --git a/spec/rubocop/cop/generator_spec.rb b/spec/rubocop/cop/generator_spec.rb index e0d8b459ffd..40bed418ecc 100644 --- a/spec/rubocop/cop/generator_spec.rb +++ b/spec/rubocop/cop/generator_spec.rb @@ -342,7 +342,8 @@ def on_send(node) expect(runner.run([])).to be true end - it 'generates a spec file that has no offense' do + # TODO: include it back after rubocop-rspec updated to support deep departments names + xit 'generates a spec file that has no offense' do generator.write_spec expect(runner.run([])).to be true end diff --git a/spec/rubocop/cop/registry_spec.rb b/spec/rubocop/cop/registry_spec.rb index fe73c800bc3..cb23d7900b5 100644 --- a/spec/rubocop/cop/registry_spec.rb +++ b/spec/rubocop/cop/registry_spec.rb @@ -172,6 +172,21 @@ it 'exposes a list of cops' do expect(registry.cops).to eql(cops) end + + context 'with cops having the same inner-most module' do + let(:cops) do + [RuboCop::Cop::Foo::Bar, RuboCop::Cop::Baz::Foo::Bar] + end + + before do + stub_const('RuboCop::Cop::Foo::Bar', Class.new(RuboCop::Cop::Base)) + stub_const('RuboCop::Cop::Baz::Foo::Bar', Class.new(RuboCop::Cop::Base)) + end + + it 'exposes both cops' do + expect(registry.cops).to match_array([RuboCop::Cop::Foo::Bar, RuboCop::Cop::Baz::Foo::Bar]) + end + end end it 'exposes the number of stored cops' do