fmt subscriber should honor NO_COLOR by default

[matklad]

https://no-color.org

Would help as a sure-way to neuter colors if you really don't want them for whatever reason.

[davidbarsky]

I'm not opposed to this change, especially since there was interest in the past to have color controlled by an env variable (#1160 (comment)). Naively, having fmt::Layer read an environment variable (e.g., NO_COLOR, but it should be overridable through a builder method) and setting fmt::Layer::with_ansi to false is reasonable, but I'm not sure whether reading this variable should be on by default or what the behavior should be if/when the user has affirmatively set with_ansi(true). What are your expectations for this?

[hawkw]

but I'm not sure whether reading this variable should be on by default or what the behavior should be if/when the user has affirmatively set with_ansi(true). What are your expectations for this?

I think the variable is intended to disable colors if it is set, but it not being set should not be considered affirmatively enabling colors. We should probably read it by default at least in all the cases where we would default to enabling ANSI colors, and disable ANSI colors if it is set.

I'm less sure what the correct behavior should be in the case where the user explicitly calls with_ansi. Open to suggestions!

[matklad]

I'm less sure what the correct behavior should be in the case where the user explicitly calls with_ansi. Open to suggestions!

My reading of no-color.org is that NO_COLOR is only intended to suppress default colors, so with_ansi should not be affected.

I guess, in the ideal world we'd do

fn with_ansi(Ansi::Yes | Ansi::No | Ansi::Auto /* based on is_term and NO_COLOR */)

but with today's API, I think the best action is not change the API in any way, and just check for NO_COLOR if with_ansi was not explicitly set.

[andrewpollack]

Looking into this issue, and I wanted to confirm. My interpretation is that setting ’NO_COLOR’ means that color is turned off (even in the case of ’with_ansi’).

Is this the desired behavior? Do other libraries/software have precedence of overwriting the ’NO_COLOR’ case?

[dmlary]

I found this issue after discovering NO_COLOR didn't work for bevy trace output

From https://no-color.org/:

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 reads as "if NO_COLOR is set to any non-empty value, the program should never output ANSI colors". This reading is based on the NO_COLOR being an accessibility flag, not a per-utility configuration.

This is counter to some of the previous comments allowing the setting to be overridden. Allowing it to be overridden a) defeats the purpose of the flag, or b) requires every library author to duplicate the code for checking NO_COLOR.

I'm happy to open a PR for this.

[dmlary]

Baaaah, I re-read the no-color website.

I found this issue after discovering NO_COLOR didn't work for bevy trace output

From https://no-color.org/:

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 reads as "if NO_COLOR is set to any non-empty value, the program should never output ANSI colors". This reading is based on the NO_COLOR being an accessibility flag, not a per-utility configuration.

Bahhhh, I re-read the website and realized later they explicitly allow overriding the NO_COLOR environment variable with a command-line flag.

Allowing it to be overridden a) defeats the purpose of the flag, or b) requires every library author to duplicate the code for checking NO_COLOR.

I'm still thinking along these lines though. We don't want the users of tracing to need to explicitly check NO_COLOR themselves. I'll update the PR to only set the default value of is_ansi based on NO_COLOR. If the caller explicitly sets ANSI with with_ansi or set_ansi, that will override the environment variable.