Extract printing concern from HookRunner into helper

The `HookRunner` class was the largest class, and arguably one of the
most complicated.

An obvious separation of concerns was the flow of events versus the
printing of output to the user. A solution was to separate the printing
into a separate helper class which the `HookRunner` could just send
messages to when certain events occurred.

It's a bit crude since we're still passing a few parameters here and
there that should probably be encapsulated in separate objects, but this
already provides us some benefit in terms of reducing the size of the
class. We also should probably consider having the printer passed to the
`HookLoader` implementations as well, but right now things still work
and this is something that can be shipped.

Change-Id: I93c639b69e4d468ac1766cf86599c413d6c482eb
Reviewed-on: http://gerrit.causes.com/40459
Tested-by: jenkins <jenkins@causes.com>
Reviewed-by: Shane da Silva <shane.dasilva@brigade.com>

Author: sds

lib/overcommit.rb

@@ -13,6 +13,7 @@
 require 'overcommit/hook_loader/built_in_hook_loader'
 require 'overcommit/hook_loader/plugin_hook_loader'
 require 'overcommit/interrupt_handler'
+require 'overcommit/printer'
 require 'overcommit/hook_runner'
 require 'overcommit/installer'
 require 'overcommit/logger'

lib/overcommit/hook_runner.rb

@@ -8,11 +8,12 @@ class HookRunner
     # @param logger [Overcommit::Logger]
     # @param context [Overcommit::HookContext]
     # @param input [Overcommit::UserInput]
-    def initialize(config, logger, context, input)
+    def initialize(config, logger, context, input, printer)
       @config = config
       @log = logger
       @context = context
       @input = input
+      @printer = printer
       @hooks = []
     end
 
@@ -33,7 +34,7 @@ def run
 
     def run_hooks
       if @hooks.any? { |hook| hook.run? || hook.skip? }
-        log.bold "Running #{hook_script_name} hooks"
+        @printer.start_run
 
         interrupted = false
         run_failed = false
@@ -50,23 +51,19 @@ def run_hooks
           end
         end
 
-        log.log # Newline
-        print_summary(run_failed, interrupted)
-        log.log # Newline
+        @printer.end_run(interrupted, run_failed)
 
         !(run_failed || interrupted)
       else
-        log.success "✓ No applicable #{hook_script_name} hooks to run"
+        @printer.nothing_to_run
         true # Run was successful
       end
     end
 
     def run_hook(hook)
       return if should_skip?(hook)
 
-      unless hook.quiet?
-        print_header(hook)
-      end
+      @printer.start_hook(hook)
 
       begin
         # Disable the interrupt handler during individual hook run so that
@@ -84,65 +81,26 @@ def run_hook(hook)
         InterruptHandler.enable!
       end
 
-      # Want to print the header in the event the result wasn't good so that the
-      # user knows what failed
-      print_header(hook) if hook.quiet? && status != :good
-
-      print_result(hook, status, output)
+      @printer.end_hook(hook, status, output)
 
       status
     end
 
-    def print_header(hook)
-      log.partial hook.description
-      log.partial '.' * (70 - hook.description.length)
-    end
-
     def should_skip?(hook)
       return true unless hook.enabled?
 
       if hook.skip?
         if hook.required?
-          log.warning "Cannot skip #{hook.name} since it is required"
+          @printer.required_hook_not_skipped
         else
-          log.warning "Skipping #{hook.name}"
+          @printer.hook_skipped
           return true
         end
       end
 
       !hook.run?
     end
 
-    def print_result(hook, status, output)
-      case status
-      when :good
-        log.success 'OK' unless hook.quiet?
-      when :warn
-        log.warning 'WARNING'
-        print_report(output, :bold_warning)
-      when :bad
-        log.error 'FAILED'
-        print_report(output, :bold_error)
-      when :interrupted
-        log.error 'INTERRUPTED'
-        print_report(output, :bold_error)
-      end
-    end
-
-    def print_report(output, format = :log)
-      log.send(format, output) unless output.nil? || output.empty?
-    end
-
-    def print_summary(run_failed, was_interrupted)
-      if was_interrupted
-        log.warning '⚠  Hook run interrupted by user'
-      elsif run_failed
-        log.error "✗ One or more #{hook_script_name} hooks failed"
-      else
-        log.success "✓ All #{hook_script_name} hooks passed"
-      end
-    end
-
     def load_hooks
       require "overcommit/hook/#{@context.hook_type_name}/base"
 
@@ -151,9 +109,5 @@ def load_hooks
       # Load plugin hooks after so they can subclass existing hooks
       @hooks += HookLoader::PluginHookLoader.new(@config, @context, @log, @input).load_hooks
     end
-
-    def hook_script_name
-      @context.hook_script_name
-    end
   end
 end

lib/overcommit/printer.rb

@@ -0,0 +1,103 @@
+module Overcommit
+  # Provide a set of callbacks which can be executed as events occur during the
+  # course of {HookRunner#run}.
+  class Printer
+    attr_reader :log
+
+    def initialize(logger, context)
+      @log = logger
+      @context = context
+    end
+
+    # Executed at the very beginning of running the collection of hooks.
+    def start_run
+      log.bold "Running #{hook_script_name} hooks"
+    end
+
+    def nothing_to_run
+      log.success "✓ No applicable #{hook_script_name} hooks to run"
+    end
+
+    # Executed at the very end of running the collection of hooks.
+    def end_run(run_failed, interrupted)
+      log.log # Newline
+
+      if interrupted
+        log.warning '⚠  Hook run interrupted by user'
+      elsif run_failed
+        log.error "✗ One or more #{hook_script_name} hooks failed"
+      else
+        log.success "✓ All #{hook_script_name} hooks passed"
+      end
+
+      log.log # Newline
+    end
+
+    # Executed at the start of an individual hook run.
+    def start_hook(hook)
+      unless hook.quiet?
+        print_header(hook)
+      end
+    end
+
+    def hook_skipped(hook)
+      log.warning "Skipping #{hook.name}"
+    end
+
+    def required_hook_not_skipped(hook)
+      log.warning "Cannot skip #{hook.name} since it is required"
+    end
+
+    # Executed at the end of an individual hook run.
+    def end_hook(hook, status, output)
+      # Want to print the header for quiet hooks only if the result wasn't good
+      # so that the user knows what failed
+      print_header(hook) if hook.quiet? && status != :good
+
+      print_result(hook, status, output)
+    end
+
+  private
+
+    def print_header(hook)
+      log.partial hook.description
+      log.partial '.' * (70 - hook.description.length)
+    end
+
+    def print_result(hook, status, output)
+      case status
+      when :good
+        log.success 'OK' unless hook.quiet?
+      when :warn
+        log.warning 'WARNING'
+        print_report(output, :bold_warning)
+      when :bad
+        log.error 'FAILED'
+        print_report(output, :bold_error)
+      when :interrupted
+        log.error 'INTERRUPTED'
+        print_report(output, :bold_error)
+      end
+    end
+
+    def print_report(output, format = :log)
+      log.send(format, output) unless output.nil? || output.empty?
+    end
+
+    def run_interrupted
+      log.warning '⚠  Hook run interrupted by user'
+    end
+
+    def run_failed
+      log.error "✗ One or more #{hook_script_name} hooks failed"
+    end
+
+    def run_succeeded
+      log.success "✓ All #{hook_script_name} hooks passed"
+    end
+
+    def hook_script_name
+      @context.hook_script_name
+    end
+  end
+end

template-dir/hooks/overcommit-hook

@@ -65,7 +65,8 @@ begin
 
   input = Overcommit::UserInput.new(STDIN)
 
-  runner = Overcommit::HookRunner.new(config, logger, context, input)
+  printer = Overcommit::Printer.new(logger, context)
+  runner = Overcommit::HookRunner.new(config, logger, context, input, printer)
 
   status = runner.run