From d5e18aef094b4f07c8f3262edd7c36b0726ae330 Mon Sep 17 00:00:00 2001 From: Rob Dupuis Date: Fri, 8 May 2015 18:56:19 +0100 Subject: [PATCH 1/5] Removed duplication between SimpleText and Pretty formatters --- lib/sshkit/all.rb | 2 +- lib/sshkit/formatters/abstract.rb | 6 ---- lib/sshkit/formatters/pretty.rb | 29 +++++++++++++------ lib/sshkit/formatters/simple_text.rb | 42 ++++++---------------------- 4 files changed, 30 insertions(+), 49 deletions(-) diff --git a/lib/sshkit/all.rb b/lib/sshkit/all.rb index 3974e32e..a4704844 100644 --- a/lib/sshkit/all.rb +++ b/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' diff --git a/lib/sshkit/formatters/abstract.rb b/lib/sshkit/formatters/abstract.rb index d339a5ea..df0a8dd4 100644 --- a/lib/sshkit/formatters/abstract.rb +++ b/lib/sshkit/formatters/abstract.rb @@ -45,12 +45,6 @@ def write(obj) end alias :<< :write - protected - - def format_std_stream_line(line) - ("\t" + line).chomp - end - private def write_at_log_level(level, messages) diff --git a/lib/sshkit/formatters/pretty.rb b/lib/sshkit/formatters/pretty.rb index 2375a964..d405945c 100644 --- a/lib/sshkit/formatters/pretty.rb +++ b/lib/sshkit/formatters/pretty.rb @@ -15,6 +15,16 @@ def write(obj) end alias :<< :write + protected + + def format_command_message(message, command, verbosity_override) + '%6s [%s] %s' % [level(verbosity_override || command.verbosity), colorize(command.uuid, :green), message] + end + + def format_log_message(log_message) + '%6s %s' % [level(log_message.verbosity), log_message.to_s] + end + private def write_command(command) @@ -27,13 +37,8 @@ def write_command(command) 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 + write_std_stream_lines(command.clear_stdout_lines, :green, command) + write_std_stream_lines(command.clear_stderr_lines, :red, command) end if command.finished? @@ -42,12 +47,18 @@ def write_command(command) end end + def write_std_stream_lines(lines, color, command) + lines.each do |line| + write_command_message(colorize(("\t" + line).chomp, color), command, Logger::DEBUG) + 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] + original_output << "#{format_command_message(message, command, verbosity_override)}\n" end def write_log_message(log_message) - original_output << "%6s %s\n" % [level(log_message.verbosity), log_message.to_s] + original_output << "#{format_log_message(log_message)}\n" end def level(verbosity) diff --git a/lib/sshkit/formatters/simple_text.rb b/lib/sshkit/formatters/simple_text.rb index 3554b66d..7e1f3fba 100644 --- a/lib/sshkit/formatters/simple_text.rb +++ b/lib/sshkit/formatters/simple_text.rb @@ -1,44 +1,20 @@ - module SSHKit module Formatter - class SimpleText < Abstract + class SimpleText < Pretty - 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 + # Historically, SimpleText formatter was used to disable coloring, so we maintain that behaviour + def colorize(obj, color, mode=nil) + obj.to_s 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 - - 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 + + def format_command_message(message, command, verbosity_override=nil) + message end - def write_log_message(log_message) - original_output << log_message.to_s + "\n" + def format_log_message(log_message) + log_message end end From 437287c6c76940d76eb8e57ad250ca3c571b1215 Mon Sep 17 00:00:00 2001 From: Rob Dupuis Date: Sat, 9 May 2015 03:55:06 +0100 Subject: [PATCH 2/5] Added tests for logging non string and leading and trailing spaces --- test/unit/formatters/test_pretty.rb | 12 +++++++++++- test/unit/formatters/test_simple_text.rb | 12 +++++++++++- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/test/unit/formatters/test_pretty.rb b/test/unit/formatters/test_pretty.rb index 8b3c4e1c..59d41afb 100644 --- a/test/unit/formatters/test_pretty.rb +++ b/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) diff --git a/test/unit/formatters/test_simple_text.rb b/test/unit/formatters/test_simple_text.rb index 20bead26..ab5b81a8 100644 --- a/test/unit/formatters/test_simple_text.rb +++ b/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') From 06f22a44677f20caf90419952576a78e3014b7d6 Mon Sep 17 00:00:00 2001 From: Rob Dupuis Date: Mon, 11 May 2015 00:04:19 +0100 Subject: [PATCH 3/5] Removed duplication in Abstract formatter --- lib/sshkit/formatters/abstract.rb | 32 +++++-------------------------- 1 file changed, 5 insertions(+), 27 deletions(-) diff --git a/lib/sshkit/formatters/abstract.rb b/lib/sshkit/formatters/abstract.rb index df0a8dd4..2ae43ceb 100644 --- a/lib/sshkit/formatters/abstract.rb +++ b/lib/sshkit/formatters/abstract.rb @@ -16,40 +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 - private - - def write_at_log_level(level, messages) - write(LogMessage.new(level, messages)) - end end end From 9a20cd5bdcc7987ad7f99514dd7de89c8e704ae8 Mon Sep 17 00:00:00 2001 From: Rob Dupuis Date: Mon, 11 May 2015 14:36:04 +0100 Subject: [PATCH 4/5] Simplified interface between Pretty and SimpleText formatter SimpleText now only needs to override one method - format_message --- lib/sshkit/formatters/pretty.rb | 61 ++++++++++++---------------- lib/sshkit/formatters/simple_text.rb | 6 +-- 2 files changed, 27 insertions(+), 40 deletions(-) diff --git a/lib/sshkit/formatters/pretty.rb b/lib/sshkit/formatters/pretty.rb index d405945c..77db195c 100644 --- a/lib/sshkit/formatters/pretty.rb +++ b/lib/sshkit/formatters/pretty.rb @@ -4,73 +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_command_message(message, command, verbosity_override) - '%6s [%s] %s' % [level(verbosity_override || command.verbosity), colorize(command.uuid, :green), message] - end - - def format_log_message(log_message) - '%6s %s' % [level(log_message.verbosity), log_message.to_s] + 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 - write_std_stream_lines(command.clear_stdout_lines, :green, command) - write_std_stream_lines(command.clear_stderr_lines, :red, command) - 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_std_stream_lines(lines, color, command) + def write_std_stream_debug(lines, color, uuid) lines.each do |line| - write_command_message(colorize(("\t" + line).chomp, color), command, Logger::DEBUG) + write_debug(colorize("\t#{line}".chomp, color), uuid) end end - def write_command_message(message, command, verbosity_override=nil) - original_output << "#{format_command_message(message, command, verbosity_override)}\n" - end - - def write_log_message(log_message) - original_output << "#{format_log_message(log_message)}\n" - end - - def level(verbosity) - colorize(level_names(verbosity), level_formatting(verbosity)) - 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 diff --git a/lib/sshkit/formatters/simple_text.rb b/lib/sshkit/formatters/simple_text.rb index 7e1f3fba..f6597bff 100644 --- a/lib/sshkit/formatters/simple_text.rb +++ b/lib/sshkit/formatters/simple_text.rb @@ -9,14 +9,10 @@ def colorize(obj, color, mode=nil) obj.to_s end - def format_command_message(message, command, verbosity_override=nil) + def format_message(verbosity, message, uuid=nil) message end - def format_log_message(log_message) - log_message - end - end end From 2aeeaccef169d5c4b86e2d3544df5097a1326e3c Mon Sep 17 00:00:00 2001 From: Rob Dupuis Date: Mon, 11 May 2015 17:14:29 +0100 Subject: [PATCH 5/5] Added note to CHANGELOG about formatter hierarchy change Also added links to PRs / Issues --- CHANGELOG.md | 41 +++++++++++++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a5e84d98..65c45d9d 100644 --- a/CHANGELOG.md +++ b/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`)