No colors for non tty outputs

[robd]

As discussed in #245, this PR reworks color support so that we don't ouput ANSI color escape sequences for output IOs which aren't associated with a tty.

The main rework is to convert the Color module into a class which can be instantiated with the output IO, rather than just being hard coded to the $stdout.tty?

As part of this change, I was trying to add a test for the colors in this message in the pretty formatter:

c.black(c.on_yellow("Output formatter doesn't know how to handle #{obj.class}\n"))

Calling write with an unexpected object type didn't call the above line, but instead raised an exception - (undefined method 'verbosity' for #<Object:0x007f8b73695450>) here, which meant that the line above was basically unreachable.

I made this line reachable by checking for the presence of the verbosity method and I saw this error instead undefined method 'on_yellow' for Color:Module.

I looked through the historical issues on SSHKit and Capistrano about the verbosity and on_yellow errors. There are several verbosity messgage issues which were was associated with the --dry-run option and I found this @leehambley comment:

I can confirm the log formatters expect objects on Command and LogMessage (?) classes. Any thing contrary is incorrect

Therefore, it seems to me that, in practice, we are never calling a formatter with a non Command / LogMessage object. Therefore, I've added tests to raise this as a more helpful error, and removed the coloring from the message.

Hopefully everything else is obvious.

[mattbrictson]

I like this but it has an unfortunate side effect of breaking airbrussh. 😭

Airbrussh constructs a pretty formatter with a Logger object, which responds to << (and thus works for output purposes). But Logger is not an IO object and does not respond to tty?. And so this happens:

NoMethodError: undefined method `tty?' for #<Logger:0x007ff88d10aaa8>
/Users/mbrictson/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/bundler/gems/sshkit-cc6fe03497d4/lib/sshkit/color.rb:15:in `colorize?'

Now this is totally my fault, because I am not passing in a proper IO object. The reason I am using Logger is because of its handy log rotation feature, which prevents the log file from growing unbounded.

Any ideas how best to solve this?

1. Change airbrussh to use open instead of Logger.new (i.e. drop the log rotation feature, or roll my own).
2. Change colorize? to use a more defensive check: @io.respond_to?(:tty?) && @io.tty?.
3. Have airbrussh extend the logger object to respond to tty?.

[robd]

Ah that is a shame - I was hoping the opposite - that this change would stop my capistrano.log file being full of color characters!

I think option 2 would be preferable. It narrows the contract for providing an output to SSHKit (ie only << or write need to be supported), and people can implement their own tty? method if they want more control (ie suggestion 3).

I think it's reasonable that if there is no tty? method we default to no color.

People providing a non IO output can wrap with a SimpleDelegator and add a tty? method.

If you're happy with this, I will add tests and update the docs to cover this case.

[mattbrictson]

@robd That works for me. Thanks! 👍

[mattbrictson]

I think this PR might also fix some lingering issues being reported in #214, since @robd's new Color implementation calls to_s before invoking colorize.

[robd]

@mattbrictson OK f37ef77 should fix the undefined method 'tty?' issue

[mattbrictson]

f37ef77 should fix the undefined method 'tty?' issue

Verified!

[leehambley]

Good work @robd and thanks again for the compat. sanity checking @mattbrictson - happy to merge if you are @robd

[robd]

@leehambley great - yes please!