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

Support NO_COLOR. #782

Merged
merged 1 commit into from
Jun 12, 2023
Merged

Support NO_COLOR. #782

merged 1 commit into from
Jun 12, 2023

Conversation

kevin-vigor
Copy link
Contributor

As a color-blind user, I frequently struggle distinguishing colorized output and appreciate the ability to disable it as per https://no-color.org/

Note that the whole parking_lot/atomic idiom to memoize the environment variable lookup is ripped off from here: https://github.com/crossterm-rs/crossterm/blob/master/src/ansi_support.rs#L33

@kevin-vigor kevin-vigor requested a review from TimonPost as a code owner April 13, 2023 17:43
@kevin-vigor kevin-vigor force-pushed the master branch 2 times, most recently from 4c84a53 to df81140 Compare April 13, 2023 21:24
@kevin-vigor kevin-vigor force-pushed the master branch 2 times, most recently from a312ec9 to fbb4842 Compare April 17, 2023 16:53
@kevin-vigor
Copy link
Contributor Author

FYI I have now updated this diff with rustfmt and validates it passes the crossterm_test workflow. Sorry for premature pull request.

@TimonPost TimonPost merged commit 2c534fc into crossterm-rs:master Jun 12, 2023
@Piturnah
Copy link
Contributor

Hello! I am the maintainer of Gex which now supports NO_COLOR, and I noticed that this PR actually breaks NO_COLOR support downstream. The reason for this becomes apparent reading the NO_COLOR FAQ:

User-level configuration files and per-instance command-line arguments should override $NO_COLOR. A user should be able to export $NO_COLOR in their shell configuration file as a default, but configure a specific program in its configuration file to specifically enable color.
This also means that software that can add color but doesn’t by default does not need to care about $NO_COLOR, because it will only ever be adding color when instructed to do so (as it should be).

With the current implementation in Crossterm, if the env variable is detected then there is no way for downstream users such as my project to override it, and as such this specification is broken.

Suggested fixes

  1. Revert the change, and allow downstream to implement NO_COLOR themselves.
  2. Add an override command to Crossterm.

Option 2 seems to make the most sense to me.

@kevin-vigor
Copy link
Contributor Author

@Piturnah : I think you are asking for a public API to override NO_COLOR?

Does kevin-vigor@5c4686c look like what you want?

@Piturnah
Copy link
Contributor

LGTM. Thanks for the quick response!

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.

3 participants