colorize icon indicating copy to clipboard operation
colorize copied to clipboard

Disable colorization by default if output is not a text terminal

Open specious opened this issue 9 years ago • 9 comments

By convention, I am used to colorized applications suppressing colorization when sending the output to a pipe, unless I explicitly force colorization.

For example, in colors.js:

if (process.stdout && !process.stdout.isTTY) {
  return false;
}

Writing an app using colorize, I was surprised that colors were not suppressed when doing app > file.txt or app | cat. The color codes can cause problems in later processing stages.

I then turned colorization off for non-TTY output with:

String.disable_colorization(true) unless STDOUT.isatty

I'd like to propose that be the default behavior, with a way to force colors by choice in all cases.

specious avatar Oct 28 '16 05:10 specious

It worked like you described, but it was changed few versions ago because of PR or issue.

fazibear avatar Oct 28 '16 13:10 fazibear

I see in the CHANGELOG:

== 0.6.0 / 2013-09-25
  ...
  * STDOUT.isatty condition removed

What were the discussions relevant to this decision?

specious avatar Oct 28 '16 21:10 specious

As you can see it was 3 years ago :) Can't find it, it was a PR or issue.

fazibear avatar Oct 29 '16 11:10 fazibear

Here's the problem. This looks nice: meetup-cli-color-output On the other hand, this is an unpleasant surprise: meetup-cli-color-codes-problem In my experience, the default behavior is usually implemented to be the opposite of what it is here.

specious avatar Oct 31 '16 21:10 specious

@specious in case this bugs you on an ongoing basis: less knows how to interpret the codes if you start it with less -r. Many other cli tools have a similar option.

If this comes back, it would sure be nice to be able to override this again (kind of like git diff --color=always) so one can choose to pipe to a log file and still get color codes.

ccoenen avatar Apr 04 '17 23:04 ccoenen

Makes sense to me to go back to at least consider going back to the previous functionality. I don't think I've ever dumped colorize strings to a log, but that's where I can think that this would be most painful, especially if said log was being read back from a log aggregator that didn't know what do with all the escape sequences.

If the PRs I submitted today look interesting to @fazibear and have a chance at getting merged in the next few months, then I might consider digging through the PR / issue history to look at when this was last working the way the issue asks in order to see if I can figure out how to restore the previous capability.

No promises, but I get why this is valuable.

(NOTE: yes, I definitely realize that this issue is now over 5 years old)

jefflunt avatar Jan 07 '22 08:01 jefflunt

I might consider digging through the PR / issue history to look at when this was last working the way the issue asks

FYI, the change was introduced in this PR: https://github.com/fazibear/colorize/pull/2

chocolateboy avatar Jan 07 '22 11:01 chocolateboy

@chocolateboy - thanks for finding that! I see this PR from the chromebrew project also mentioned the change to this gem, and that they re-introduced the TTY-sensitivity on their own.

What's your take on #2 and the reason for removing this originally?

I created a branch with this change on my fork to test.

jefflunt avatar Jan 07 '22 15:01 jefflunt

I think #2 should be opt-in (like --color=always in ripgrep). It's a reasonable feature ("An option to colorize output when writing to a file"), but the implementation has inadvertently introduced a bug ("Doesn't disable color when writing to a file").

chocolateboy avatar Jan 07 '22 15:01 chocolateboy