Refactor Rubocop runner into more objects

Code Climate triggered a warning that this class was too large and
looking closely it seems to have several different responsibilities
mixed together. At the same time, a lot of the language used in this
engine doesn't match the language of how we talk about our domain.

These changes aim to break down the Rubocop class into collaborators
and to refine the language to bring its concerns closer to our
domain language.

* Introduce SourceFile
* Rename ViolationDecorator to Issue
* Move presentation concerns into Issue
* Refine remediation point names to better match what they represent
* Begin to improve coverage on remediation point generation

Author: jpignata

config/cops.yml

@@ -1,25 +1,25 @@
 Metrics/AbcSize:
   base_points: 1_000_000
-  violation_points: 70_000
+  overage_points: 70_000
 Metrics/BlockNesting:
-  base_points: 100_000
+  remediation_points: 100_000
 Metrics/ClassLength:
   base_points: 5_000_000
-  violation_points: 35_000
-Metrics/CyclomaticComplexity: # This check is per method
+  overage_points: 35_000
+Metrics/CyclomaticComplexity:
   base_points: 1_000_000
-  violation_points: 70_000
+  overage_points: 70_000
 Metrics/LineLength:
-  base_points: 50_000
+  remediation_points: 50_000
 Metrics/MethodLength:
   base_points: 1_000_000
-  violation_points: 70_000
+  overage_points: 70_000
 Metrics/ModuleLength:
   base_points: 5_000_000
-  violation_points: 35_000
+  overage_points: 35_000
 Metrics/ParameterList:
   base_points: 500_000
-  violation_points: 100_000
+  overage_points: 100_000
 Metrics/PerceivedComplexity:
   base_points: 1_000_000
-  violation_points: 70_000
+  overage_points: 70_000

lib/cc/engine/file_list_resolver.rb

@@ -1,11 +1,11 @@
 module CC
   module Engine
     class FileListResolver
-      def initialize(code:, engine_config: {}, rubocop_config_store:)
-        @code = code
+      def initialize(root:, engine_config: {}, config_store:)
+        @root = root
         @exclude_paths = engine_config["exclude_paths"] || []
         @include_paths = engine_config["include_paths"]
-        @rubocop_config_store = rubocop_config_store
+        @config_store = config_store
       end
 
       def expanded_list
@@ -39,21 +39,21 @@ def include_based_files_to_inspect
       end
 
       def local_path(path)
-        realpath = Pathname.new(@code).realpath.to_s
+        realpath = Pathname.new(@root).realpath.to_s
         path.gsub(%r{^#{realpath}/}, '')
       end
 
       def rubocop_file_to_include?(file)
         if file =~ /\.rb$/
           true
         else
-          dir, basename = File.split(file)
-          @rubocop_config_store.for(dir).file_to_include?(basename)
+          root, basename = File.split(file)
+          @config_store.for(root).file_to_include?(basename)
         end
       end
 
       def rubocop_runner
-        @rubocop_runner ||= RuboCop::Runner.new({}, @rubocop_config_store)
+        @rubocop_runner ||= RuboCop::Runner.new({}, @config_store)
       end
     end
   end

lib/cc/engine/issue.rb

@@ -0,0 +1,125 @@
+require 'safe_yaml'
+SafeYAML::OPTIONS[:default_mode] = :safe
+
+module CC
+  module Engine
+    class Issue < SimpleDelegator
+      MULTIPLIER_REGEX = %r{\[([\d\.]+)\/([\d\.]+)\]}
+      DEFAULT_REMEDIATION_POINTS = 50_000
+      DEFAULT_BASE_POINTS = 200_000
+      DEFAULT_OVERAGE_POINTS = 50_000
+
+      def initialize(issue, path, cop_list: nil)
+        @path = path
+        @cop_list = cop_list
+        super(issue)
+      end
+
+      # rubocop:disable Metrics/MethodLength
+      def to_json
+        hash = {
+          type: "Issue",
+          check_name: "Rubocop/#{cop_name}",
+          description: message,
+          categories: [category],
+          remediation_points: remediation_points,
+          location: {
+            path: path,
+            positions: positions,
+          },
+        }
+        hash.merge!(content: { body: content_body }) if content_body.present?
+        hash.to_json
+      end
+
+      def remediation_points
+        if multiplier?
+          base_points + overage_points
+        else
+          cop_definition.fetch("remediation_points", DEFAULT_REMEDIATION_POINTS)
+        end
+      end
+
+      private
+
+      attr_reader :path
+
+      def multiplier?
+        message.match(MULTIPLIER_REGEX)
+      end
+
+      def base_points
+        cop_definition.fetch("base_points", DEFAULT_BASE_POINTS)
+      end
+
+      def cop_definition
+        @cop_definition ||= cop_list.fetch(cop_name, {})
+      end
+
+      def cop_list
+        @cop_list ||= YAML.load_file(expand_config_path("cops.yml"))
+      end
+
+      def expand_config_path(path)
+        File.expand_path("../../../../config/#{path}", __FILE__)
+      end
+
+      def overage_points
+        overage_points = cop_definition.
+          fetch("overage_points", DEFAULT_OVERAGE_POINTS)
+
+        overage_points * multiplier
+      end
+
+      def multiplier
+        result = message.scan(MULTIPLIER_REGEX)
+        score, max = result[0]
+        score.to_i - max.to_i
+      end
+
+      def category
+        CategoryParser.new(cop_name).category
+      end
+
+      def positions
+        {
+          begin: {
+            column: columns.first,
+            line: lines.first,
+          },
+          end: {
+            column: columns.last,
+            line: lines.last,
+          }
+        }
+      end
+
+      # Increment column value as columns are 0-based in parser
+      def columns
+        return @columns if defined?(@columns)
+
+        if location.is_a?(RuboCop::Cop::Lint::Syntax::PseudoSourceRange)
+          @columns = [location.column + 1, location.column + 1]
+        else
+          @columns = [location.column + 1, location.last_column + 1]
+        end
+      end
+
+      def lines
+        return @lines if defined?(@lines)
+
+        if location.is_a?(RuboCop::Cop::Lint::Syntax::PseudoSourceRange)
+          @lines = [location.line, location.line]
+        else
+          @lines = [location.first_line, location.last_line]
+        end
+      end
+
+      def content_body
+        return @content_body if defined?(@content_body)
+        content_path = expand_config_path("contents/#{cop_name.underscore}.md")
+        @content_body = File.exist?(content_path) && File.read(content_path)
+      end
+    end
+  end
+end

lib/cc/engine/rubocop.rb

@@ -4,130 +4,54 @@
 require "rubocop/cop/method_complexity_patch"
 require "rubocop/cop/method_length"
 require "rubocop/cop/class_length"
+require "cc/engine/source_file"
 require "cc/engine/category_parser"
 require "cc/engine/file_list_resolver"
-require "cc/engine/violation_decorator"
+require "cc/engine/issue"
 require "active_support"
 require "active_support/core_ext"
 
 module CC
   module Engine
     class Rubocop
-      def initialize(code, engine_config, io)
-        @code = code
+      def initialize(root, engine_config, io)
+        @root = root
         @engine_config = engine_config || {}
         @io = io
       end
 
       def run
-        Dir.chdir(@code) do
+        Dir.chdir(root) do
           files_to_inspect.each do |path|
-            inspect_file(path)
+            SourceFile.new(
+              config_store: config_store.for(path),
+              io: io,
+              path: path,
+              root: root,
+            ).inspect
           end
         end
       end
 
       private
 
-      def files_to_inspect
-        @file_list_resolver = FileListResolver.new(
-          code: @code,
-          engine_config: @engine_config,
-          rubocop_config_store: rubocop_config_store
-        )
-        @file_list_resolver.expanded_list
-      end
-
-      def category(cop_name)
-        CategoryParser.new(cop_name).category
-      end
-
-      def inspect_file(path)
-        ruby_version = target_ruby_version(path)
-        parsed = RuboCop::ProcessedSource.from_file(path, ruby_version)
-        rubocop_team_for_path(path).inspect_file(parsed).each do |violation|
-          next if violation.disabled?
-          decorated_violation = ViolationDecorator.new(violation)
-          json = violation_json(decorated_violation, local_path(path))
-          @io.print "#{json}\0"
-        end
-      end
+      attr_reader :root, :engine_config, :io
 
-      def local_path(path)
-        realpath = Pathname.new(@code).realpath.to_s
-        path.gsub(%r{^#{realpath}/}, '')
+      def files_to_inspect
+        @files_to_inspect ||= FileListResolver.new(
+          config_store: config_store,
+          engine_config: engine_config,
+          root: root,
+        ).expanded_list
       end
 
-      def rubocop_config_store
-        @rubocop_config_store ||= begin
-          Dir.chdir(@code) do
-            config_store = RuboCop::ConfigStore.new
-            if (config_file = @engine_config["config"])
-              config_store.options_config = config_file
-            end
-            config_store
+      def config_store
+        @config_store ||= RuboCop::ConfigStore.new.tap do |config_store|
+          if (config_file = engine_config["config"])
+            config_store.options_config = config_file
           end
         end
       end
-
-      def rubocop_team_for_path(path)
-        rubocop_config = rubocop_config_store.for(path)
-        RuboCop::Cop::Team.new(RuboCop::Cop::Cop.all, rubocop_config)
-      end
-
-      def target_ruby_version(path)
-        config_store = rubocop_config_store.for(path)
-        config_store["AllCops"] && config_store["AllCops"]["TargetRubyVersion"]
-      end
-
-      def violation_positions(location)
-        if location.is_a?(RuboCop::Cop::Lint::Syntax::PseudoSourceRange)
-          first_line = location.line
-          last_line = location.line
-          first_column = location.column
-          last_column = location.column
-        else
-          first_line = location.first_line
-          last_line = location.last_line
-          first_column = location.column
-          last_column = location.last_column
-        end
-
-        {
-          begin: {
-            column: first_column + 1, # columns are 0-based in Parser
-            line: first_line,
-          },
-          end: {
-            column: last_column + 1,
-            line: last_line,
-          }
-        }
-      end
-
-      def violation_json(violation, local_path)
-        violation_hash = {
-          type: "Issue",
-          check_name: "Rubocop/#{violation.cop_name}",
-          description: violation.message,
-          categories: [category(violation.cop_name)],
-          remediation_points: violation.remediation_points,
-          location: {
-            path: local_path,
-            positions: violation_positions(violation.location),
-          },
-        }
-        body = content_body(violation.cop_name)
-        violation_hash.merge!(content: { body: body }) if body.present?
-        violation_hash.to_json
-      end
-
-      def content_body(cop_name)
-        path = File.expand_path(
-          "../../../../config/contents/#{cop_name.underscore}.md", __FILE__
-        )
-        File.exist?(path) ? File.read(path) : nil
-      end
     end
   end
 end

lib/cc/engine/source_file.rb

@@ -0,0 +1,42 @@
+module CC
+  module Engine
+    class SourceFile
+      def initialize(config_store:, io:, path:, root:)
+        @config_store = config_store
+        @io = io
+        @path = path
+        @root = root
+      end
+
+      def inspect
+        rubocop_team.inspect_file(processed_source).each do |offense|
+          next if offense.disabled?
+
+          io.print Issue.new(offense, display_path).to_json
+          io.print "\0"
+        end
+      end
+
+      private
+
+      attr_reader :config_store, :io, :path, :root
+
+      def processed_source
+        RuboCop::ProcessedSource.from_file(path, target_ruby_version)
+      end
+
+      def target_ruby_version
+        config_store["AllCops"] && config_store["AllCops"]["TargetRubyVersion"]
+      end
+
+      def rubocop_team
+        RuboCop::Cop::Team.new(RuboCop::Cop::Cop.all, config_store)
+      end
+
+      def display_path
+        realpath = Pathname.new(root).realpath.to_s
+        path.gsub(%r{^#{realpath}/}, "")
+      end
+    end
+  end
+end