crossterm-rs / crossterm

Cross platform terminal library rust
MIT License
3.29k stars 280 forks source link

Support NO_COLOR. #782

Closed kevin-vigor closed 1 year ago

kevin-vigor commented 1 year ago

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 commented 1 year ago

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

Piturnah commented 1 year ago

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 commented 1 year ago

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

Does https://github.com/kevin-vigor/crossterm/commit/5c4686c0b901d3cdff91ce99a5e369f36eab02ec look like what you want?

Piturnah commented 1 year ago

LGTM. Thanks for the quick response!