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

Tty: if NO_COLOR env var is present, disable color #3090

Merged
merged 1 commit into from Feb 12, 2018

Conversation

Projects
None yet
4 participants
@jcs
Copy link
Contributor

jcs commented Aug 26, 2017

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew tests with your changes locally?

If the NO_COLOR environment variable is set, even with a valid TTY, disable color output.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 27, 2017

Given there's literally no software on that list supporting this variable it's pretty far from being a standard so 👎 to merging this until this standard is much further along.

@reitermarkus

This comment has been minimized.

Copy link
Member

reitermarkus commented Aug 27, 2017

Why not just change your terminal theme? You don‘t need an environment variable and it works for any software.

@jcs

This comment has been minimized.

Copy link
Contributor

jcs commented Aug 27, 2017

Homebrew is also the only software on that list that has no command line flag or other option to disable colors (short of piping every command to a non-tty), so I'm starting with Homebrew.

@reitermarkus Because certain programs (like vim) are configured to use colors, but with particular themes and then only sparingly. I'm not trying to remove color completely. Command-line programs outputting colored text without being told to is a relatively new phenomenon.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Aug 27, 2017

Homebrew is also the only software on that list that has no command line flag or other option to disable colors

It's a list of four items so I'm not surprised. Given this site/standard is something you've created yourself in the last two days and has no significant widespread adoption yet: passing on this for now, sorry.

@raindev

This comment has been minimized.

Copy link

raindev commented Feb 12, 2018

@MikeMcQuaid, even if NO_COLOR is a non-standard why not allow users to disable colored output?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Feb 12, 2018

@raindev I gave my reasons above and this PR is from almost 6 months ago when the situation was different.

@MikeMcQuaid MikeMcQuaid reopened this Feb 12, 2018

@MikeMcQuaid
Copy link
Member

MikeMcQuaid left a comment

You'll also need to add NO_COLOR to

brew/bin/brew

Line 50 in b6a1302

BROWSER EDITOR GIT PATH VISUAL \
(which will pass it through to homebrew as HOMEBREW_NO_COLOR), document HOMEBREW_NO_COLOR in https://github.com/Homebrew/brew/blob/master/Library/Homebrew/manpages/brew.1.md.erb#L100 and run brew man and commit the output.

@@ -56,7 +56,7 @@ def reset_escape_sequence!
end

def to_s
return "" unless $stdout.tty?
return "" unless $stdout.tty? && !ENV["NO_COLOR"]

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Feb 12, 2018

Member

Change this into an if (two, if necessary) and check the HOMEBREW_NO_COLOR variable instead (this will still accept NO_COLOR but be converted elsewhere).

@jcs

This comment has been minimized.

Copy link
Contributor

jcs commented Feb 12, 2018

I rebased to HEAD and made the requested changes, but it fails testing because of Library/Homebrew/test/cask/dsl_spec.rb:77. I'm not sure how you want to handle that test.

@MikeMcQuaid
Copy link
Member

MikeMcQuaid left a comment

fails testing because of Library/Homebrew/test/cask/dsl_spec.rb:77. I'm not sure how you want to handle that test.

Add another allow(ENV).to line below here: https://github.com/Homebrew/brew/blob/master/Library/Homebrew/test/cask/dsl_spec.rb#L77

@@ -59,7 +59,9 @@ def reset_escape_sequence!
end

def to_s
return "" unless $stdout.tty?
if ENV["HOMEBREW_NO_COLOR"] || !$stdout.tty?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Feb 12, 2018

Member

If this fits on one line <= 80 characters then you can do that otherwise: no worries as-is.

expect(subject.magenta.to_s).to eq("")
expect(subject.cyan.to_s).to eq("")
expect(subject.default.to_s).to eq("")
ENV.delete "HOMEBREW_NO_COLOR"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Feb 12, 2018

Member

Can skip this because it's done automatically

Tty: if HOMEBREW_NO_COLOR env var is present, disable color
bin/brew will recognize NO_COLOR variable and copy it to
HOMEBREW_NO_COLOR
@jcs

This comment has been minimized.

Copy link
Contributor

jcs commented Feb 12, 2018

👍

@MikeMcQuaid MikeMcQuaid merged commit b3f0e57 into Homebrew:master Feb 12, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Feb 12, 2018

Thanks so much for your first contribution, @jcs! Without people like you submitting PRs we couldn't run this project. You rock!

@jcs

This comment has been minimized.

Copy link
Contributor

jcs commented Feb 12, 2018

Thanks for reconsidering and helping this get merged Mike

@raindev

This comment has been minimized.

Copy link

raindev commented Feb 12, 2018

I'm happy to see how it turned out ♥️

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Feb 13, 2018

@jcs No problem; I saw this on Hacker News the other day and figured after that if we had more people ask about it then we could accept it now it's a bit more established. Good luck with the project!

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Feb 13, 2018

@jcs One other thought: you may want to have this set HOMEBREW_NO_EMOJI too?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Feb 27, 2018

@jcs Note: this is in v1.5.5 which was just released.

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.