Experimental: refactor Airbrussh::Colors to be class, rather than module

[mattbrictson]

This refactor more closely aligns Airbrussh::Colors with SSHKit::Color. Now they are both classes and their colorization behavior is toggled based on a flag (which is determined using :tty?).

This will bring us closer to consolidating the colors implementation across projects.

What do you think of this so far, @robd ?

[robd]

Looks good to me.

I like the pattern of constructing a no-op Colors class with the enabled attribute as opposed to the output one passed in SSHKit.

How do you feel about it?

[mattbrictson]

I feel like it is not quite as elegant as applying colors everywhere and then deferring the color/no color decision until the final output in Console.

Also, this refactor is a slight change in behavior: if SSH command output contains color, Console used to strip the color if color was disabled. Now it just passes through the output as-is. I'm not sure which behavior is "correct", but I think the original behavior is more desirable.

I might tinker with this some more to restore the original Console behavior but retain the refactoring.

[robd]

Also, this refactor is a slight change in behavior...

OK github just lost my comment here, but to summarise briefly, this change in behaviour was what I was trying to get at here: #63 (comment)

One difference is that in the SSHKit approach, colors wouldn't be stripped from output of other commands called by SSHKit. I can't work out if this is a benefit or disadvantage. If I understand correctly, Airbrussh's config.color = false option strips the colors from the output of commands run by SSHKit. This could be useful if users want the formatter to do this but I'm not sure if that is a common use case

Since we haven't released a version of SSHKit with colors disabled for non tty outputs, I don't have a strong feeling for whether what we currently do in SSHKit is desirable for users. Ideally I think it would be good if SSHKit and Airbrussh could work the same. I think it would be fine to change the SSHKit behaviour to strip the colours at the end perhaps by moving the console class (or whatever is responsible for line length limiting and colors) to SSHKit. But I'd probably only look to do this if we were going to merge the projects (in fact I would merge them first).

I'm aware that this is all probably going in a direction you're not very happy with, so please don't feel like you have to do this refactor on my account. I think it has been useful to throw up these differences between Airbrussh / SSHKit, we can refer here if support requests are raised about coloring differences once Airbrussh is the default formatter.

[mattbrictson]

Yes, this is quite tricky, because perfectly aligning how Airbrussh and the other formatters work will require a lot of subtle changes. There are also concepts that don't really line up at all, like output_verbosity, which Pretty uses but Airbrussh ignores in favor of the more fine-grained command_output setting.

And I agree that this is probably not worth the effort unless we merge the code bases. However, I want to experiment with the idea just to see what's involved.

One more idea is to not perfectly unify the logging configuration for all formatters. Instead, take a declarative, middleware-style approach where each formatter can be given its own config.

Something like this:

SSHKit.configure do |config|
  config.use_format :pretty, :color => false, :verbosity => :debug
end

Or for Airbrussh:

SSHKit.configure do |config|
  config.use_format :airbrussh, :command_output => true, :truncate => :auto
end

A symbol would be constantized e.g. :pretty → SSHKit::Formatter::Pretty, but if you want to write a custom formatter outside the SSHKit namespace, you could pass in the class itself:

SSHKit.configure do |config|
  config.use_format MyCustomFormatter, :custom_option => true
end

This would allow a lot of flexibility without having to unify the configuration.

Thoughts?

[robd]

I think this is really nice. I think per-formatter options would be very helpful. An immediate example that springs to mind is that this would allow an easy path for removing the SimpleText subclass:

SSHKit.configure do |config|
  config.use_format :pretty, :simple_text => true
end

[mattbrictson]

@leehambley I know this is a little off-topic, but what do you think of a new SSHKit formatter configuration system as outlined in my comment above?

[leehambley]

I need to get a release out today (Autodesk are relying on using the new
licensed version in something they're shipping to customers -
On 29 Oct 2015 8:00 p.m., "Matt Brictson" notifications@github.com wrote:

@leehambley https://github.com/leehambley I know this is a little
off-topic, but what do you think of a new SSHKit formatter configuration
system as outlined in my comment above
#64 (comment)
?

—
Reply to this email directly or view it on GitHub
#64 (comment)
.

[leehambley]

But I'm not opposed, no strong opinion, and anything I could argue would be
bike shedding.
On 30 Oct 2015 10:42 a.m., "Lee Hambley" lee.hambley@gmail.com wrote:

I need to get a release out today (Autodesk are relying on using the new
licensed version in something they're shipping to customers -
On 29 Oct 2015 8:00 p.m., "Matt Brictson" notifications@github.com
wrote:

@leehambley https://github.com/leehambley I know this is a little
off-topic, but what do you think of a new SSHKit formatter configuration
system as outlined in my comment above
#64 (comment)
?

—
Reply to this email directly or view it on GitHub
#64 (comment)
.

[mattbrictson]

@robd Would you agree this PR is good to merge? I've restored the color-stripping behavior to the Console class, so the changes are really just about making Colors a class rather than a module.

[mattbrictson]

@robd On second thought, I'm still not sure I want to go ahead with this refactor. I am going to let this simmer so more.

[mattbrictson]

Closing. See comment: #63 (comment)