Better separation of concerns between Team and Commissioner

lib/rubocop/cop/commissioner.rb

@@ -36,7 +36,6 @@ def initialize(cops, forces = [], options = {})
 
       def investigate(processed_source)
         reset_errors
-        remove_irrelevant_cops(processed_source.file_path)
         reset_callbacks
         prepare(processed_source)
         invoke_custom_processing(@cops, processed_source)
@@ -63,26 +62,6 @@ def reset_errors
         @errors = []
       end
 
-      def remove_irrelevant_cops(filename)
-        @cops.reject! do |cop|
-          cop.excluded_file?(filename) ||
-            !support_target_ruby_version?(cop) ||
-            !support_target_rails_version?(cop)
-        end
-      end
-
-      def support_target_ruby_version?(cop)
-        return true unless cop.class.respond_to?(:support_target_ruby_version?)
-
-        cop.class.support_target_ruby_version?(cop.target_ruby_version)
-      end
-
-      def support_target_rails_version?(cop)
-        return true unless cop.class.respond_to?(:support_target_rails_version?)
-
-        cop.class.support_target_rails_version?(cop.target_rails_version)
-      end
-
       def reset_callbacks
         @callbacks.clear
       end

lib/rubocop/cop/team.rb

@@ -11,12 +11,12 @@ class Team
 
       Investigation = Struct.new(:offenses, :errors)
 
-      attr_reader :errors, :warnings, :updated_source_file
+      attr_reader :errors, :warnings, :updated_source_file, :cops
 
       alias updated_source_file? updated_source_file
 
-      def initialize(cop_classes, config, options = nil)
-        @cop_classes = cop_classes
+      def initialize(cops, config = nil, options = nil)
+        @cops = cops
         @config = config
         @options = options || DEFAULT_OPTIONS
         @errors = []
@@ -25,6 +25,23 @@ def initialize(cop_classes, config, options = nil)
         validate_config
       end
 
+      # @return [Team] with cops assembled from the given `cop_classes`
+      def self.mobilize(cop_classes, config, options = nil)
+        options ||= DEFAULT_OPTIONS
+        cops = mobilize_cops(cop_classes, config, options)
+        new(cops, config, options)
+      end
+
+      # @return [Array<Cop::Cop>]
+      def self.mobilize_cops(cop_classes, config, options = nil)
+        options ||= DEFAULT_OPTIONS
+        only = options.fetch(:only, [])
+        safe = options.fetch(:safe, false)
+        cop_classes.enabled(config, only, safe).map do |cop_class|
+          cop_class.new(config, options)
+        end
+      end
+
       def autocorrect?
         @options[:auto_correct]
       end
@@ -44,14 +61,6 @@ def inspect_file(processed_source)
         offenses(processed_source)
       end
 
-      def cops
-        only = @options.fetch(:only, [])
-        safe = @options.fetch(:safe, false)
-        @cops ||= @cop_classes.enabled(@config, only, safe).map do |cop_class|
-          cop_class.new(@config, @options)
-        end
-      end
-
       def forces
         @forces ||= forces_for(cops)
       end
@@ -93,21 +102,23 @@ def external_dependency_checksum
 
       private
 
-      def offenses(processed_source)
+      def offenses(processed_source) # rubocop:disable Metrics/AbcSize
         # The autocorrection process may have to be repeated multiple times
         # until there are no corrections left to perform
         # To speed things up, run auto-correcting cops by themselves, and only
         # run the other cops when no corrections are left
-        autocorrect_cops, other_cops = cops.partition(&:autocorrect?)
-
-        autocorrect =
-          investigate(autocorrect_cops, processed_source) do |offenses|
-            # We corrected some errors. Another round of inspection will be
-            # done, and any other offenses will be caught then, so we don't
-            # need to continue.
-            return offenses if autocorrect(processed_source.buffer,
-                                           autocorrect_cops)
-          end
+        on_duty = roundup_relevant_cops(processed_source.file_path)
+
+        autocorrect_cops, other_cops = on_duty.partition(&:autocorrect?)
+
+        autocorrect = investigate(autocorrect_cops, processed_source)
+
+        if autocorrect(processed_source.buffer, autocorrect_cops)
+          # We corrected some errors. Another round of inspection will be
+          # done, and any other offenses will be caught then, so we don't
+          # need to continue.
+          return autocorrect.offenses
+        end
 
         other = investigate(other_cops, processed_source)
 
@@ -120,13 +131,32 @@ def offenses(processed_source)
       def investigate(cops, processed_source)
         return Investigation.new([], {}) if cops.empty?
 
-        commissioner = Commissioner.new(cops, forces_for(cops))
+        commissioner = Commissioner.new(cops, forces_for(cops), @options)
         offenses = commissioner.investigate(processed_source)
-        yield offenses if block_given?
 
         Investigation.new(offenses, commissioner.errors)
       end
 
+      def roundup_relevant_cops(filename)
+        cops.reject do |cop|
+          cop.excluded_file?(filename) ||
+            !support_target_ruby_version?(cop) ||
+            !support_target_rails_version?(cop)
+        end
+      end
+
+      def support_target_ruby_version?(cop)
+        return true unless cop.class.respond_to?(:support_target_ruby_version?)
+
+        cop.class.support_target_ruby_version?(cop.target_ruby_version)
+      end
+
+      def support_target_rails_version?(cop)
+        return true unless cop.class.respond_to?(:support_target_rails_version?)
+
+        cop.class.support_target_rails_version?(cop.target_rails_version)
+      end
+
       def autocorrect_all_cops(buffer, cops)
         corrector = Corrector.new(buffer)
 

lib/rubocop/rspec/cop_helper.rb

@@ -64,16 +64,8 @@ def autocorrect_source_with_loop(source, file = nil)
   end
 
   def _investigate(cop, processed_source)
-    forces = RuboCop::Cop::Force.all.each_with_object([]) do |klass, instances|
-      next unless cop.join_force?(klass)
-
-      instances << klass.new([cop])
-    end
-
-    commissioner =
-      RuboCop::Cop::Commissioner.new([cop], forces, raise_error: true)
-    commissioner.investigate(processed_source)
-    commissioner
+    team = RuboCop::Cop::Team.new([cop], nil, raise_error: true)
+    team.inspect_file(processed_source)
   end
 end
 

lib/rubocop/runner.rb

@@ -183,7 +183,7 @@ def filtered_run?
     def autocorrect_redundant_disables(file, source, cop, offenses)
       cop.processed_source = source
 
-      team = Cop::Team.new(RuboCop::Cop::Registry.new, nil, @options)
+      team = Cop::Team.mobilize(RuboCop::Cop::Registry.new, nil, @options)
       team.autocorrect(source.buffer, [cop])
 
       return [] unless team.updated_source_file?
@@ -295,7 +295,7 @@ def check_for_infinite_loop(processed_source, offenses)
 
     def inspect_file(processed_source)
       config = @config_store.for(processed_source.path)
-      team = Cop::Team.new(mobilized_cop_classes(config), config, @options)
+      team = Cop::Team.mobilize(mobilized_cop_classes(config), config, @options)
       offenses = team.inspect_file(processed_source)
       @errors.concat(team.errors)
       @warnings.concat(team.warnings)
@@ -380,7 +380,7 @@ def get_processed_source(file)
     def standby_team(config)
       @team_by_config ||= {}
       @team_by_config[config.object_id] ||=
-        Cop::Team.new(mobilized_cop_classes(config), config, @options)
+        Cop::Team.mobilize(mobilized_cop_classes(config), config, @options)
     end
   end
 end

spec/rubocop/cop/commissioner_spec.rb

@@ -2,106 +2,71 @@
 
 RSpec.describe RuboCop::Cop::Commissioner do
   describe '#investigate' do
+    subject(:offenses) { commissioner.investigate(processed_source) }
+
     let(:cop) do
       # rubocop:disable RSpec/VerifiedDoubles
-      double(RuboCop::Cop::Cop, offenses: [],
-                                excluded_file?: false).as_null_object
+      double(RuboCop::Cop::Cop, offenses: []).as_null_object
       # rubocop:enable RSpec/VerifiedDoubles
     end
-    let(:force) { instance_double(RuboCop::Cop::Force).as_null_object }
+    let(:cops) { [cop] }
+    let(:options) { {} }
+    let(:forces) { [] }
+    let(:commissioner) { described_class.new(cops, forces, **options) }
+    let(:errors) { commissioner.errors }
+    let(:source) { <<~RUBY }
+      def method
+      1
+      end
+    RUBY
+    let(:processed_source) { parse_source(source, 'file.rb') }
 
     it 'returns all offenses found by the cops' do
       allow(cop).to receive(:offenses).and_return([1])
 
-      commissioner = described_class.new([cop], [])
-      source = ''
-      processed_source = parse_source(source)
-
-      expect(commissioner.investigate(processed_source)).to eq [1]
-    end
-
-    context 'when a cop has no interest in the file' do
-      it 'returns all offenses except the ones of the cop' do
-        cops = []
-        cops << instance_double(RuboCop::Cop::Cop, offenses: %w[foo],
-                                                   excluded_file?: false)
-        cops << instance_double(RuboCop::Cop::Cop, excluded_file?: true)
-        cops << instance_double(RuboCop::Cop::Cop, offenses: %w[baz],
-                                                   excluded_file?: false)
-        cops.each(&:as_null_object)
-
-        commissioner = described_class.new(cops, [])
-        source = ''
-        processed_source = parse_source(source)
-
-        expect(commissioner.investigate(processed_source)).to eq %w[foo baz]
-      end
+      expect(offenses).to eq [1]
     end
 
     it 'traverses the AST and invoke cops specific callbacks' do
       expect(cop).to receive(:on_def).once
-
-      commissioner = described_class.new([cop], [])
-      source = <<~RUBY
-        def method
-        1
-        end
-      RUBY
-      processed_source = parse_source(source)
-
-      commissioner.investigate(processed_source)
-    end
-
-    it 'passes the input params to all cops/forces that implement their own' \
-       ' #investigate method' do
-      source = ''
-      processed_source = parse_source(source)
-
-      expect(cop).to receive(:investigate).with(processed_source)
-      expect(force).to receive(:investigate).with(processed_source)
-
-      commissioner = described_class.new([cop], [force])
-
-      commissioner.investigate(processed_source)
+      offenses
     end
 
     it 'stores all errors raised by the cops' do
       allow(cop).to receive(:on_int) { raise RuntimeError }
 
-      commissioner = described_class.new([cop], [])
-      source = <<~RUBY
-        def method
-        1
-        end
-      RUBY
-      processed_source = parse_source(source)
-
-      commissioner.investigate(processed_source)
-
-      expect(commissioner.errors.size).to eq(1)
+      expect(offenses).to eq []
+      expect(errors.size).to eq(1)
       expect(
-        commissioner.errors[0].cause.instance_of?(RuntimeError)
+        errors[0].cause.instance_of?(RuntimeError)
       ).to be(true)
-      expect(commissioner.errors[0].line).to eq 2
-      expect(commissioner.errors[0].column).to eq 0
+      expect(errors[0].line).to eq 2
+      expect(errors[0].column).to eq 0
     end
 
     context 'when passed :raise_error option' do
+      let(:options) { { raise_error: true } }
+
       it 're-raises the exception received while processing' do
         allow(cop).to receive(:on_int) { raise RuntimeError }
 
-        commissioner = described_class.new([cop], [], raise_error: true)
-        source = <<~RUBY
-          def method
-          1
-          end
-        RUBY
-        processed_source = parse_source(source)
-
         expect do
-          commissioner.investigate(processed_source)
+          offenses
         end.to raise_error(RuntimeError)
       end
     end
+
+    context 'when given a force' do
+      let(:force) { instance_double(RuboCop::Cop::Force).as_null_object }
+      let(:forces) { [force] }
+
+      it 'passes the input params to all cops/forces that implement their own' \
+         ' #investigate method' do
+        expect(cop).to receive(:investigate).with(processed_source)
+        expect(force).to receive(:investigate).with(processed_source)
+
+        offenses
+      end
+    end
   end
 end

spec/rubocop/cop/layout/leading_empty_lines_spec.rb

@@ -104,7 +104,7 @@ def bar(arg =1); end
         RUBY
 
         options = { auto_correct: true, stdin: true }
-        team = RuboCop::Cop::Team.new(cops, config, options)
+        team = RuboCop::Cop::Team.mobilize(cops, config, options)
         team.inspect_file(parse_source(source_with_offenses, nil))
         new_source = options[:stdin]
 

spec/rubocop/cop/team_spec.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 RSpec.describe RuboCop::Cop::Team do
-  subject(:team) { described_class.new(cop_classes, config, options) }
+  subject(:team) { described_class.mobilize(cop_classes, config, options) }
 
   let(:cop_classes) { RuboCop::Cop::Cop.registry }
   let(:config) { RuboCop::ConfigLoader.default_configuration }
@@ -122,6 +122,16 @@ def a
       it 'returns offenses from cops' do
         expect(cop_names).to include('Layout/LineLength')
       end
+
+      context 'when a cop has no interest in the file' do
+        it 'returns all offenses except the ones of the cop' do
+          allow_any_instance_of(RuboCop::Cop::Layout::LineLength)
+            .to receive(:excluded_file?).and_return(true)
+
+          expect(cop_names).to include('Lint/AmbiguousOperator')
+          expect(cop_names).not_to include('Layout/LineLength')
+        end
+      end
     end
 
     context 'when autocorrection is enabled' do
@@ -389,7 +399,7 @@ def external_dependency_checksum
 
       it 'has a different checksum for the whole team' do
         original_checksum = team.external_dependency_checksum
-        new_team = described_class.new(new_cop_classes, config, options)
+        new_team = described_class.mobilize(new_cop_classes, config, options)
         new_checksum = new_team.external_dependency_checksum
         expect(original_checksum).not_to eq(new_checksum)
       end