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

Force color #2449

Merged
merged 8 commits into from
Aug 17, 2022
Merged

Force color #2449

merged 8 commits into from
Aug 17, 2022

Conversation

darrenburns
Copy link
Member

@darrenburns darrenburns commented Aug 5, 2022

Type of changes

  • Bug fix
  • New feature
  • Documentation / docstrings
  • Tests
  • Other

Checklist

  • I've run the latest black with default args on new code.
  • I've updated CHANGELOG.md and CONTRIBUTORS.md where appropriate.
  • I've added tests for new code.
  • I accept that @willmcgugan may be pedantic in the code review.

Description

Adds support for FORCE_COLOR environment variable.
Presence of this variable in the environment results in Console.force_terminal = True.
If FORCE_COLOR and NO_COLOR are both set, NO_COLOR takes priority.

NO_COLOR = 1 only

image

NO_COLOR = 1 and FORCE_COLOR = 1

image

FORCE_COLOR = 1 only

image

@codecov-commenter
Copy link

codecov-commenter commented Aug 5, 2022

Codecov Report

Merging #2449 (0162d17) into master (19e518f) will decrease coverage by 0.11%.
The diff coverage is 98.38%.

@@            Coverage Diff             @@
##           master    #2449      +/-   ##
==========================================
- Coverage   98.71%   98.59%   -0.12%     
==========================================
  Files          73       72       -1     
  Lines        7771     7769       -2     
==========================================
- Hits         7671     7660      -11     
- Misses        100      109       +9     
Flag Coverage Δ
unittests 98.59% <98.38%> (-0.12%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
rich/default_styles.py 100.00% <ø> (ø)
rich/logging.py 100.00% <ø> (ø)
rich/progress.py 92.76% <ø> (ø)
rich/scope.py 100.00% <ø> (ø)
rich/segment.py 98.72% <93.10%> (+<0.01%) ⬆️
rich/_inspect.py 100.00% <100.00%> (ø)
rich/box.py 100.00% <100.00%> (ø)
rich/cells.py 96.05% <100.00%> (-3.95%) ⬇️
rich/color.py 100.00% <100.00%> (ø)
rich/console.py 97.67% <100.00%> (-0.62%) ⬇️
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@darrenburns darrenburns marked this pull request as ready for review August 9, 2022 10:57
@darrenburns darrenburns linked an issue Aug 16, 2022 that may be closed by this pull request
@darrenburns
Copy link
Member Author

@willmcgugan would appreciate a review here when you get a chance.

@willmcgugan willmcgugan merged commit b8a2cf0 into master Aug 17, 2022
@willmcgugan willmcgugan deleted the force-color branch August 17, 2022 13:52
@hugovk
Copy link
Contributor

hugovk commented Sep 16, 2022

Thank you for this!

Do you have a rough idea of when the next release is?

Would be great to get this released, integrated and tested into pip in time for their next release some time on October.

@willmcgugan
Copy link
Collaborator

Potentially as soon as this weekend.

@henryiii
Copy link
Contributor

henryiii commented Oct 2, 2022

And it’s out! <3

@webknjaz
Copy link

(discoverability comment) PSA: looks like using NO_COLOR for overriding FORCE_COLOR is currently broken:

@henryiii
Copy link
Contributor

I believe the problem is that this is handled unusually: if both NO_COLOR and FORCE_COLOR are set, instead of avoiding ANSI escapes, it instead makes everything black and white, but still uses ANSI escapes for things like bold text. (Personally, I prefer controlling color by the value of FORCE_COLOR; in plumbum I use the value for the color mode).

@willmcgugan
Copy link
Collaborator

I asked the author of the NO_COLOR spec, and he confirmed that NO_COLOR wasn't intended as a strip all escape sequences switch. i.e. bold / italic etc isn't considered color.

@hugovk
Copy link
Contributor

hugovk commented Nov 21, 2023

@henryiii
Copy link
Contributor

I’m not sure that’s the correct reference. That one says monochrome is still color. :)

A big part of the problem is you can’t set NO_COLOR to an empty string or some sort of false value (that’s part of the spec). Unsetting an environment value is sometimes hard (there’s no “none” in TOML, for example, which means tools like cibuildwheel and scikit-build-core can’t undo a set variable via static config since there’s no way to indicate it). But that’s part of the spec, not much that can be done.

@webknjaz
Copy link

I asked the author of the NO_COLOR spec, and he confirmed that NO_COLOR wasn't intended as a strip all escape sequences switch. i.e. bold / italic etc isn't considered color.

Honestly, reading https://github.com/jcs/no_color/issues/240 I interpreted it differently..

Also, I don't think that “bold” is a good example, since some terminals don't implement it properly and abuse it for making the color brighter. I hit this with Kitty, which implements the standard to the letter and when some apps misuse it, they sometimes ends up printing “invisible” color (as in black on black).

@webknjaz
Copy link

I’m not sure that’s the correct reference. That one says monochrome is still color. :)

A big part of the problem is you can’t set NO_COLOR to an empty string or some sort of false value (that’s part of the spec). Unsetting an environment value is sometimes hard (there’s no “none” in TOML, for example, which means tools like cibuildwheel and scikit-build-core can’t undo a set variable via static config since there’s no way to indicate it). But that’s part of the spec, not much that can be done.

Exactly, in my case, it's YAML, which does have None through ~. But the context is GHA where you can only override environment variables, but not unset them on the workflow level.

@webknjaz
Copy link

@willmcgugan so it looks like the combination of env vars like FORCE_COLOR= NO_COLOR=1 ends up forcing non-color ANSI escape codes.

One would expect that NO_COLOR would neuter FORCE_COLOR (as in, has the exact opposite effect). But in practice, the latter forces color and non-color ANSI-codes and the former only cancels out the color ANSI-codes.

This brings up a question — which ANSI-codes constitute color? You mentioned bold/italic being non-color, so why does FORCE_COLOR implies enabling them, then?

https://force-color.org suggests that FORCE_COLOR should enable color ANSI escape codes, but it seems like Rich does more than that.

Command-line software which outputs colored text should check for a FORCE_COLOR environment variable. When this variable is present and not an empty string (regardless of its value), it should force the addition of ANSI color.

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

Successfully merging this pull request may close these issues.

[REQUEST] Enable colours when FORCE_COLOR env var is set
6 participants