Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable colorization by default if output is not a text terminal #53

Open
specious opened this issue Oct 28, 2016 · 9 comments
Open

Disable colorization by default if output is not a text terminal #53

specious opened this issue Oct 28, 2016 · 9 comments
Assignees

Comments

@specious
Copy link

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.

@fazibear
Copy link
Owner

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

@specious
Copy link
Author

I see in the CHANGELOG:

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

What were the discussions relevant to this decision?

@fazibear
Copy link
Owner

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

@specious
Copy link
Author

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.

@ccoenen
Copy link

ccoenen commented Apr 4, 2017

@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.

@jefflunt
Copy link
Contributor

jefflunt commented Jan 7, 2022

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)

@chocolateboy
Copy link

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: #2

@jefflunt
Copy link
Contributor

jefflunt commented Jan 7, 2022

@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.

@chocolateboy
Copy link

chocolateboy commented Jan 7, 2022

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").

@fazibear fazibear self-assigned this Dec 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants