Skip to content

Commit

Permalink
Respect the use of ENV['NO_COLOR']
Browse files Browse the repository at this point in the history
Respect the declaration of the of the NO_COLOR environment variable as a sign
that color output should be suppressed in he tool. This informal
standard is described at https://no-color.org
  • Loading branch information
coderjoe committed Oct 6, 2019
1 parent 1a2d64d commit 21e6568
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 1 deletion.
6 changes: 5 additions & 1 deletion lib/thor/shell/color.rb
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,11 @@ def set_color(string, *colors)
protected

def can_display_colors?
stdout.tty?
stdout.tty? && !are_colors_disabled?
end

def are_colors_disabled?
!ENV['NO_COLOR'].nil?

This comment has been minimized.

Copy link
@nobu

nobu Aug 5, 2022

Contributor

This returns true even if NO_COLOR is set to an empty string.

https://no-color.org proposes:

Command-line software which adds ANSI color to its output by default
should check for a NO_COLOR environment variable that, when present
and not an empty string (regardless of its value), prevents the
addition of ANSI color.

This comment has been minimized.

Copy link
@matthewd

matthewd Aug 5, 2022

Member

Oh neat, the "standard" quietly changed its definition from present-but-empty explicitly meaning it was set ("regardless of its value") to now meaning not-set, seemingly because it's easier to implement that way in shell syntax?!

jcs/no_color@99f90e2

I'm not sure how much sense it makes to chase the whims of an unversioned and apparently-open-to-change document. (It lists a lot of software that already claimed to support its standard before it was flipped... I wonder how many are now suddenly non-compliant?)

This comment has been minimized.

Copy link
@nobu

nobu Aug 6, 2022

Contributor

Thanks for the link.
I didn't notice that it changed.

Sure, novice shell programmers may not know ${var+set} syntax.
I think https://github.com/jcs/no_color/issues/57#issuecomment-674162146 would be another reason.

This comment has been minimized.

Copy link
@coderjoe

coderjoe Aug 7, 2022

Author Contributor

Though somewhat annoying it seems valuable enough that I think it's worth fixing. I'll submit an issue for the problem and a PR will follow shortly.

This comment has been minimized.

Copy link
@coderjoe

coderjoe Aug 7, 2022

Author Contributor

PR #796 submitted

Lets move any additional discussion there.

This comment has been minimized.

Copy link
@coderjoe

coderjoe Aug 10, 2022

Author Contributor

PR #796 has been merged. NO_COLOR support will consider empty string as not-set in the next release.

end

# Overwrite show_diff to show diff with colors if Diff::LCS is
Expand Down
25 changes: 25 additions & 0 deletions spec/shell/color_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,15 @@ def shell
shell.ask "Is this green?", :green, :limited_to => %w(Yes No Maybe)
end

it "does not set the color if specified and NO_COLOR is set" do
allow(ENV).to receive(:[]).with("NO_COLOR").and_return("")
expect(Thor::LineEditor).to receive(:readline).with("Is this green? ", anything).and_return("yes")
shell.ask "Is this green?", :green

expect(Thor::LineEditor).to receive(:readline).with("Is this green? [Yes, No, Maybe] ", anything).and_return("Yes")
shell.ask "Is this green?", :green, :limited_to => %w(Yes No Maybe)
end

it "handles an Array of colors" do
expect(Thor::LineEditor).to receive(:readline).with("\e[32m\e[47m\e[1mIs this green on white? \e[0m", anything).and_return("yes")
shell.ask "Is this green on white?", [:green, :on_white, :bold]
Expand Down Expand Up @@ -48,6 +57,15 @@ def shell
expect(out.chomp).to eq("Wow! Now we have colors!")
end

it "does not set the color if NO_COLOR is set" do
allow(ENV).to receive(:[]).with("NO_COLOR").and_return("")
out = capture(:stdout) do
shell.say "Wow! Now we have colors!", :green
end

expect(out.chomp).to eq("Wow! Now we have colors!")
end

it "does not use a new line even with colors" do
out = capture(:stdout) do
shell.say "Wow! Now we have colors! ", :green
Expand Down Expand Up @@ -118,6 +136,13 @@ def shell
colorless = shell.set_color "hi!", :white
expect(colorless).to eq("hi!")
end

it "does nothing when the terminal has the NO_COLOR environment variable set" do
allow(ENV).to receive(:[]).with("NO_COLOR").and_return("")
allow($stdout).to receive(:tty?).and_return(true)
colorless = shell.set_color "hi!", :white
expect(colorless).to eq("hi!")
end
end

describe "#file_collision" do
Expand Down

0 comments on commit 21e6568

Please sign in to comment.