Proposal - Replace SimpleText formatter with SSHKit.config.log_colors = false option

[robd]

@leehambley I didn't want to push a PR on you, but looking at the discussion in #120, you mentioned that you thought is was bit of a shame to add more formatter logic when the SimpleText formatter was added, and I agree. That PR is a bit out of date now that term-ansicolor was replaced with colorize, but if I understand correctly, the SimpleText formatter is just the Pretty formatter with colors turned off.

It seems to me that whether or not you want coloring could be orthogonal to which formatter you use. Certainly, there seems to be a lot of duplication between SimpleText and Pretty, which makes refactoring in this area harder.

I was wondering if you would be in favour of deprecating the SimpleText formatter and introducing a SSHKit.config.log_colors (or similar) option.

The implementation for this seems simple - we could just include a check for SSHKit.config.log_colors here. For the moment, we could keep the SimpleText class but delete the duplicated write and write_command methods. Then we could add a new constructor like this:

class SimpleText < Pretty
  def initialize(output)
    SSHKit.config.log_colors = false
    # (I will come up with better deprecation wording here about how to migrate the various options)
    output << 'The SimpleText formatter is now deprecated, and will be deleted soon....'
  end
end

This would have the advantage of being able to disable colors on the Dot formatter.

Of course I would update the docs, changelog and tests as usual.

[leehambley]

but if I understand correctly, the SimpleText formatter is just the Pretty formatter with colors turned off.

Absolutely

I was wondering if you would be in favour of deprecating the SimpleText formatter and introducing a SSHKit.config.log_colors (or similar) option.

Maybe extending that check also to honour not just $stdout.tty? but also ENV['CLICOLOR'] in some combination ($stdout.tty? to set the default value of those env vars?)?

[mattbrictson]

Doesn't SSHKit already honor ENV["SSHKIT_COLOR"]?

sshkit/lib/sshkit/color.rb

Line 24 in db4d1d3

ENV['SSHKIT_COLOR'] || $stdout.tty?

[robd]

@mattbrictson It does, but the problem is that at the moment you can't force the coloring to be off if stdout.tty? is true. One approach would be to support SSHKIT_COLOR=false by changing the logic to:

def colorize?
  ($stdout.tty? && ENV['SSHKIT_COLOR'] != false) || ENV['SSHKIT_COLOR']
end

Alternatively, If I was to implement CLICOLOR support I would do it as suggested on that page.

As I understand the ramifications would be as follows (are we happy with these?):

The default behaviour of the Pretty formatter may change to not color for some people (if their environment doesn't provide CLICOLOR). If they want colors back they need to specify CLICOLOR if their environment isn't providing it or CLICOLOR_FORCE if stdout doesn't have a tty.

If someone has colors who doesn't want them, they must have the CLICOLOR set, so to fix this, they should remove CLICOLOR from their ENV.

I guess we should deprecate SSHKIT_COLOR too and warn if people are using this (simple solution is just to replace with CLICOLOR_FORCE).

[leehambley]

Agreed all round. I could even agree to keep SSHKIT_COLOR, for some
imaginary use cases where someone is using sshkit in a pipeline and maybe
wants our colour, but nothing from other things in the pipe. We might also
consider (does it maybe even exist already?) If capistrano could take a
--colour option. Replying via email on my phone, so I'm not really sure
On 6 May 2015 7:54 pm, "Rob Dupuis" [email protected] wrote:

@mattbrictson https://github.com/mattbrictson It does, but the problem
is that at the moment you can't force the coloring to be off if
stdout.tty? is true. One approach would be to support SSHKIT_COLOR=false
this logic to:

def colorize?
($stdout.tty? && ENV['SSHKIT_COLOR'] != false) || ENV['SSHKIT_COLOR']end

Alternatively, If I was to implement CLICOLOR support I would do it as
suggested on that page http://bixense.com/clicolors/#how-to-implement.

As I understand the ramifications would be as follows (are we happy with
these?):

The default behaviour of the Pretty formatter may change to not color for
some people (if their environment doesn't provide CLICOLOR). If they want
colors back they need to specify CLICOLOR if their environment isn't
providing it or CLICOLOR_FORCE if stdout doesn't have a tty.

If someone has colors who doesn't want them, they must have the CLICOLOR
set, so to fix this, they should remove CLICOLOR from their ENV.

I guess we should deprecate SSHKIT_COLOR too and warn if people are using
this (simple solution is just to replace with CLICOLOR_FORCE).

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

[mattbrictson]

Hmm, I think it is more reasonable to just assume the user will want color output, as long as there is a tty. We shouldn't require a environment variable to be set to enable color. This is actually the first I've ever heard of CLICOLOR.

It seems most tools these days default to color: git, npm, brew, and bundle all give me color output, and I never had to set export CLICOLOR=1 in my profile.

The default behaviour of the Pretty formatter may change to not color for some people (if their environment doesn't provide CLICOLOR). If they want colors back they need to specify CLICOLOR if their environment isn't providing it

I think this problem is going to be widespread. CLICOLOR is not set by the Mac Terminal, for example, even though it defaults to TERM=xterm-256color.

Perhaps we could take the rainbow gem's approach (code) and enable color by default, but disable if ENV["TERM"] == "dumb"? I realize this is exactly the hack that CLICOLOR is trying to avoid, but color by default seems the more desirable behavior, even if this means hacks are required.

[robd]

OK, I guess this will need rework tomorrow, but here is a branch with the changes necessary to support CLICOLOR:

robd/sshkit@clarify-output-option...robd:remove-formatter-duplication

I do agree though, if this isn't widely supported, then I think it will be a big support hassle to get everyone to add environment variables when they upgrade.

If we're not going to go with a (semi) standard naming such as CLICOLOR or ENV["TERM"] == "dumb", another env var I thought about supporting is SSHKIT_NOCOLOR.

One thing that I discovered is that SimpleText isn't just Pretty with colors turned off. It removes some of the preamble when writing out commands and log messages, so I'm not sure that deprecating it (as I was suggesting before) is the right thing to do. I was able to remove the duplication between Pretty and SimpleText and maintain the no colors behavior of SimpleText. But one question is, should we change the default for SimpleText to be colored? This would require everyone who set up SimpleText to add in the new ENV var eg ENV["TERM"] = "dumb" or ENV["SSHKIT_NOCOLOR"] = "True".

One change in the above branch is that I now use the tty? of the IO stream which is being written to by the formatter rather than of $stdout. This should mean colors are disabled when writing to files, which would be an improvement IMO.

If we can come to a consensus on how this should work, I'll rework the PR tomorrow.

[leehambley]

Hmm, I think it is more reasonable to just assume the user will want color output, as long as there is a tty. We shouldn't require a environment variable to be set to enable color. This is actually the first I've ever heard of CLICOLOR.

CLICOLOR isn't just some hack that that guy with the site dreamt up, it has a reasonable and lengthy history, some 3rd party source on the topic at SO.

As I've seen it, let's take any given CI server, and suggest that it doesn't force a tty, or that it's something being scripted in a bash script, but driven with GNU expect, in both these cases (in the latter example, bash is connected to a tty, but the program under expect, is not). Here to force colours to be enabled in spite of the lack of a tty, the environment variable could be exported.

I'm against the ENV['TERM'] == "dumb", as I've never seen that anywhere before and a cursory google shows people referring to "dumb" terminals, but not that "dumb" is some kind of magic name to disable colouring.

The TERM env var is mostly important to termcap more than to decide on colours, whilst there might be some overlap, you can have a xterm-256-color TERM, but be running a programme without a tty (for some scripting reasons) and it'll prefer not to use colours as identified by libtermcap (or similar), but want to force colour via the env.

I'd say a good default, respecting widely used env vars (I've also never heard of CLICOLOR_FORCE as linked in that page), and having our own env var as a forceable override. From the outside then, we can add a flag to Capistrano to enable it to be set via the Ruby API which should override everything.

In pseudo code… ?

ColouringEnabled = ENV.fetch("CLICOLOR", $stdout.tty?)
ColouringEnabled = ENV.fetch("SSHKIT_COLOR", ColoringEnabled)

and, then later it can be forced by Capistrano since we might have done this init when setting up an SSHKit instance already?

[robd]

Hey all

Thanks for your involvement and feedback here. I'm going to look at this again now and I'd like to start with solving a smaller part of the problem if that's OK. I'm getting a bit confused and the changeset is quite big, so I think splitting it down would help make more incremental progress.

The first thing I'd like to change is that we respect the tty? of the actual stream being used.

At the moment, for the following configuration, you get color codes in the output file:

SSHKit.config.output = SSHKit::Formatter::Pretty.new(File.open('log/deploy.log', 'wb'))

That is because the logic we've been talking about in the colorize? method checks $stdout.tty? (which is true) instead of the tty? of the actual IO being used (in this case File.open('log/deploy.log', 'wb')) (which is false).

I propose to rework things so that the tty of IO output stream being used is checked in colorize?.

@leehambley & @mattbrictson, can I check that this would be a desirable change and you are happy with this as a first step?

[leehambley]

Logical, good deductions
On 8 May 2015 1:23 pm, "Rob Dupuis" [email protected] wrote:

Hey all

Thanks for your involvement and feedback here. I'm going to look at this
again now and I'd like to start with solving a smaller part of the problem
if that's OK. I'm getting a bit confused and the changeset is quite big, so
I think splitting it down would help make more incremental progress.

The first thing I'd like to change is that we respect the tty? of the
actual stream being used.

At the moment, for the following configuration, you get color codes in the
output file:

SSHKit.config.output = SSHKit::Formatter::Pretty.new(File.open('log/deploy.log', 'wb'))

That is because the logic we've been talking about in the colorize? method

sshkit/lib/sshkit/color.rb

Line 24 in db4d1d3

ENV['SSHKIT_COLOR'] || $stdout.tty?

checks $stdout.tty? (which is true) instead of the tty? of the actual IO
being used (in this case File.open('log/deploy.log', 'wb')) (which is
false).

I propose to rework things so that the tty of IO output stream being used
is checked in colorize?.

@leehambley https://github.com/leehambley & @mattbrictson
https://github.com/mattbrictson, can I check that this would be a
desirable change and you are happy with this as a first step?

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

[mattbrictson]

I propose to rework things so that the tty of IO output stream being used is checked in colorize?

I agree this is a good first step.

[robd]

OK great. See what you think of #246.