Remove formatter duplication

CHANGELOG.md

@@ -7,15 +7,40 @@ appear at the top.
 
   * Add your entries below here, remember to credit yourself however you want
     to be credited!
-  * Now only color the output if it is associated with a tty, or the `SSHKIT_COLOR` environment variable is set. (See [README](README.md#output-colors)) @robd
-  * Removed broken support for assigning an `IO` to the `output` config option (See [#243](https://github.com/capistrano/sshkit/issues/243)). @robd
+  * Simplified formatter hierarchy.
+    [PR #248](https://github.com/capistrano/sshkit/pull/248)
+    @robd
+    * `SimpleText` formatter now extends `Pretty`, rather than duplicating.
+  * Hide ANSI color escape sequences when outputting to a file.
+    [README](README.md#output-colors),
+    [Issue #245](https://github.com/capistrano/sshkit/issues/245),
+    [PR #246](https://github.com/capistrano/sshkit/pull/246)
+    @robd
+    * Now only color the output if it is associated with a tty,
+      or the `SSHKIT_COLOR` environment variable is set.
+  * Removed broken support for assigning an `IO` to the `output` config option.
+    [Issue #243](https://github.com/capistrano/sshkit/issues/243),
+    [PR #244](https://github.com/capistrano/sshkit/pull/244)
+    @robd
     * Use `SSHKit.config.output = SSHKit::Formatter::SimpleText.new($stdin)` instead
-  * Added support for :interaction_handler option on commands. @robd
-  * Removed partially supported 'trace' log level. @robd
-  * No longer strip whitespace or newlines in `capture` method on Netssh backend. @robd
-    * This is to make the `Local` and `Netssh` backends consistent (they diverged at 7d15a9a)
-    * If you need the old behaviour back, call `.strip` (or `.chomp`) on the captured string i.e. `capture(:my_command).strip`
-  * Simplified backend hierarchy. @robd
+  * Added support for `:interaction_handler` option on commands.
+    [PR #234](https://github.com/capistrano/sshkit/pull/234),
+    [PR #242](https://github.com/capistrano/sshkit/pull/242)
+    @robd
+  * Removed partially supported `TRACE` log level.
+    [2aa7890](https://github.com/capistrano/sshkit/commit/2aa78905f0c521ad9f697e7a4ed04ba438d5ee78)
+    @robd
+  * No longer strip whitespace or newlines in `capture` method on Netssh backend.
+    [PR #239](https://github.com/capistrano/sshkit/pull/239)
+    @robd
+    * This is to make the `Local` and `Netssh` backends consistent
+      (they diverged at [7d15a9a](https://github.com/capistrano/sshkit/commit/7d15a9aebfcc43807c8151bf6f3a4bc038ce6218))
+    * If you need the old behaviour back, call `.strip` (or `.chomp`) on the captured string
+      i.e. `capture(:my_command).strip`
+  * Simplified backend hierarchy.
+    [PR #235](https://github.com/capistrano/sshkit/pull/235),
+    [PR #237](https://github.com/capistrano/sshkit/pull/237)
+    @robd
     * Moved duplicate implementations of `make`, `rake`, `test`, `capture`, `background` on to `Abstract` backend.
     * Backend implementations now only need to implement `execute_command`, `upload!` and `download!`
     * Removed `Printer` from backend hierarchy for `Local` and `Netssh` backends (they now just extend `Abstract`)

lib/sshkit/all.rb

@@ -18,8 +18,8 @@
 
 require_relative 'formatters/abstract'
 require_relative 'formatters/black_hole'
-require_relative 'formatters/simple_text'
 require_relative 'formatters/pretty'
+require_relative 'formatters/simple_text'
 require_relative 'formatters/dot'
 
 require_relative 'runners/abstract'

lib/sshkit/formatters/abstract.rb

@@ -16,46 +16,18 @@ def initialize(output)
         @color = SSHKit::Color.new(output)
       end
 
-      def log(messages)
-        info(messages)
-      end
-
-      def fatal(messages)
-        write_at_log_level(Logger::FATAL, messages)
-      end
-
-      def error(messages)
-        write_at_log_level(Logger::ERROR, messages)
-      end
-
-      def warn(messages)
-        write_at_log_level(Logger::WARN, messages)
-      end
-
-      def info(messages)
-        write_at_log_level(Logger::INFO, messages)
-      end
-
-      def debug(messages)
-        write_at_log_level(Logger::DEBUG, messages)
+      %w(fatal error warn info debug).each do |level|
+        define_method(level) do |message|
+          write(LogMessage.new(Logger.const_get(level.upcase), message))
+        end
       end
+      alias :log :info
 
       def write(obj)
         raise "Abstract formatter should not be used directly, maybe you want SSHKit::Formatter::BlackHole"
       end
       alias :<< :write
 
-      protected
-
-      def format_std_stream_line(line)
-        ("\t" + line).chomp
-      end
-
-      private
-
-      def write_at_log_level(level, messages)
-        write(LogMessage.new(level, messages))
-      end
     end
 
   end

lib/sshkit/formatters/pretty.rb

@@ -4,62 +4,64 @@ module Formatter
 
     class Pretty < Abstract
 
+      LEVEL_NAMES = %w{ DEBUG INFO WARN ERROR FATAL }.freeze
+      LEVEL_COLORS = [:black, :blue, :yellow, :red, :red].freeze
+
       def write(obj)
         return if obj.respond_to?(:verbosity) && obj.verbosity < SSHKit.config.output_verbosity
         case obj
         when SSHKit::Command    then write_command(obj)
-        when SSHKit::LogMessage then write_log_message(obj)
+        when SSHKit::LogMessage then write_message(obj.verbosity, obj.to_s)
         else
-          raise "Output formatter only supports formatting SSHKit::Command and SSHKit::LogMessage, called with #{obj.class}: #{obj.inspect}"
+          raise "Output formatter only supports formatting SSHKit::Command and SSHKit::LogMessage, " \
+                "called with #{obj.class}: #{obj.inspect}"
         end
       end
       alias :<< :write
 
+      protected
+
+      def format_message(verbosity, message, uuid=nil)
+        message = "[#{colorize(uuid, :green)}] #{message}" unless uuid.nil?
+        level = colorize(Pretty::LEVEL_NAMES[verbosity], Pretty::LEVEL_COLORS[verbosity])
+        '%6s %s' % [level, message]
+      end
+
       private
 
       def write_command(command)
+        uuid = command.uuid
+
         unless command.started?
           host_prefix = command.host.user ? "as #{colorize(command.host.user, :blue)}@" : 'on '
-          write_command_message("Running #{colorize(command, :yellow, :bold)} #{host_prefix}#{colorize(command.host, :blue)}", command)
-          if SSHKit.config.output_verbosity == Logger::DEBUG
-            write_command_message("Command: #{colorize(command.to_command, :blue)}", command, Logger::DEBUG)
-          end
+          message = "Running #{colorize(command, :yellow, :bold)} #{host_prefix}#{colorize(command.host, :blue)}"
+          write_message(command.verbosity, message, uuid)
+          write_debug("Command: #{colorize(command.to_command, :blue)}", uuid)
         end
 
-        if SSHKit.config.output_verbosity == Logger::DEBUG
-          command.clear_stdout_lines.each do |line|
-            write_command_message(colorize(format_std_stream_line(line), :green), command, Logger::DEBUG)
-          end
-
-          command.clear_stderr_lines.each do |line|
-            write_command_message(colorize(format_std_stream_line(line), :red), command, Logger::DEBUG)
-          end
-        end
+        write_std_stream_debug(command.clear_stdout_lines, :green, uuid)
+        write_std_stream_debug(command.clear_stderr_lines, :red, uuid)
 
         if command.finished?
+          runtime = sprintf('%5.3f seconds', command.runtime)
           successful_or_failed =  command.failure? ? colorize('failed', :red, :bold) : colorize('successful', :green, :bold)
-          write_command_message("Finished in #{sprintf('%5.3f seconds', command.runtime)} with exit status #{command.exit_status} (#{successful_or_failed}).", command)
+          message = "Finished in #{runtime} with exit status #{command.exit_status} (#{successful_or_failed})."
+          write_message(command.verbosity, message, uuid)
         end
       end
 
-      def write_command_message(message, command, verbosity_override=nil)
-        original_output << "%6s [%s] %s\n" % [level(verbosity_override || command.verbosity), colorize(command.uuid, :green), message]
-      end
-
-      def write_log_message(log_message)
-        original_output << "%6s %s\n" % [level(log_message.verbosity), log_message.to_s]
-      end
-
-      def level(verbosity)
-        colorize(level_names(verbosity), level_formatting(verbosity))
+      def write_std_stream_debug(lines, color, uuid)
+        lines.each do |line|
+          write_debug(colorize("\t#{line}".chomp, color), uuid)
+        end
       end
 
-      def level_formatting(level_num)
-        [:black, :blue, :yellow, :red, :red][level_num]
+      def write_debug(message, uuid)
+        write_message(Logger::DEBUG, message, uuid) if SSHKit.config.output_verbosity == Logger::DEBUG
       end
 
-      def level_names(level_num)
-        %w{ DEBUG INFO WARN ERROR FATAL }[level_num]
+      def write_message(verbosity, message, uuid=nil)
+        original_output << "#{format_message(verbosity, message, uuid)}\n"
       end
 
     end

lib/sshkit/formatters/simple_text.rb

@@ -1,44 +1,16 @@
-
 module SSHKit
 
   module Formatter
 
-    class SimpleText < Abstract
-
-      def write(obj)
-        return if obj.respond_to?(:verbosity) && obj.verbosity < SSHKit.config.output_verbosity
-        case obj
-        when SSHKit::Command    then write_command(obj)
-        when SSHKit::LogMessage then write_log_message(obj)
-        else
-          raise "Output formatter only supports formatting SSHKit::Command and SSHKit::LogMessage, called with #{obj.class}: #{obj.inspect}"
-        end
-      end
-      alias :<< :write
-
-      private
-
-      def write_command(command)
-        unless command.started?
-          original_output << "Running #{String(command)} #{command.host.user ? "as #{command.host.user}@" : "on "}#{command.host}\n"
-          if SSHKit.config.output_verbosity == Logger::DEBUG
-            original_output << "Command: #{command.to_command}" + "\n"
-          end
-        end
-
-        if SSHKit.config.output_verbosity == Logger::DEBUG
-          (command.clear_stdout_lines + command.clear_stderr_lines).each do |line|
-            original_output << format_std_stream_line(line) << "\n"
-          end
-        end
+    class SimpleText < Pretty
 
-        if command.finished?
-          original_output << "Finished in #{sprintf('%5.3f seconds', command.runtime)} with exit status #{command.exit_status} (#{ command.failure? ? 'failed' : 'successful' }).\n"
-        end
+      # Historically, SimpleText formatter was used to disable coloring, so we maintain that behaviour
+      def colorize(obj, color, mode=nil)
+        obj.to_s
       end
 
-      def write_log_message(log_message)
-        original_output << log_message.to_s + "\n"
+      def format_message(verbosity, message, uuid=nil)
+        message
       end
 
     end

test/unit/formatters/test_pretty.rb

@@ -55,10 +55,20 @@ def test_command_lifecycle_logging_with_color
     }.each do |level, expected_output|
       define_method("test_#{level}_output_without_color") do
         pretty.send(level, "Test")
-        assert_equal expected_output, output
+        assert_log_output expected_output
       end
     end
 
+    def test_logging_message_with_leading_and_trailing_space
+      pretty.log("       some spaces\n\n  \t")
+      assert_log_output "  INFO some spaces\n"
+    end
+
+    def test_can_log_non_strings
+      pretty.log(Pathname.new('/var/log/my.log'))
+      assert_log_output "  INFO /var/log/my.log\n"
+    end
+
     def test_command_lifecycle_logging_without_color
       simulate_command_lifecycle(pretty)
 

test/unit/formatters/test_simple_text.rb

@@ -19,10 +19,20 @@ def simple
     %w(fatal error warn info debug).each do |level|
       define_method("test_#{level}_output") do
         simple.send(level, 'Test')
-        assert_equal "Test\n", output
+        assert_log_output "Test\n"
       end
     end
 
+    def test_logging_message_with_leading_and_trailing_space
+      simple.log("       some spaces\n\n  \t")
+      assert_log_output "some spaces\n"
+    end
+
+    def test_can_log_non_strings
+      simple.log(Pathname.new('/var/log/my.log'))
+      assert_log_output "/var/log/my.log\n"
+    end
+
     def test_command_lifecycle_logging
       command = SSHKit::Command.new(:a_cmd, 'some args', host: Host.new('user@localhost'))
       command.stubs(:uuid).returns('aaaaaa')