Empty NO_COLOR environmental variable should not disable coloring

[FRex]

Currently the NO_COLOR variable is handled with this code (in 2 places) - env::var_os("NO_COLOR").is_some().

The correct version (only disable coloring when the variable is set and value is not an empty string) of this check would be env::var_os("NO_COLOR").unwrap_or_default().len() > 0 or similar, possibly in a function (to not duplicate it) with appropriate explanation/rationale and link to https://no-color.org/ as well.

I can prepare a PR with a commit like that if you agree and wish me to do that.

The wording on https://no-color.org/ is clear this is the intention, but this is a very recent change to the wording. It was done after ripgrep and many other programs added this functionality: jcs/no_color@99f90e2

I assume it was clarified to make it easier for shell scripts and some scripting languages to handle, and to allow enabling color for single command using NO_COLOR= that-command (I tried this today with ripgrep and it failed to enable color).

The no-color author's comment https://github.com/jcs/no_color/issues/83#issuecomment-1166731293 also says this was original intended meaning, so it was essentially ambiguity in the original wording, which lead to many projects checking for just presence.

[BurntSushi]

So someone already pointed this out and submitted a patch to fix it, which I closed, because it's very clearly a breaking change: #63

This is exactly why I regret ever adding support for NO_COLOR in the first place. The spec was poorly worded. And now folks like us maintaining code that others rely are left holding the bag.

Regardless of what the "spirit" of the spec was, the way I implemented it matched what the spec said. I really just don't see it changing at this point. It will break real users that did export NO_COLOR=. And when it breaks them, they're going to be like, "what the fuck." Because the failure mode ain't going to be obvious.

I think the right path forward is to deprecate NO_COLOR and create a new spec that subsumes all use cases. NO_COLOR is basically now a buggy spec, because there are two different ways in the wild to implement it.

[FRex]

Yes, I also was surprised to find this and think the spec is buggy. Empty string and unset are not the same, even in bash, the original wording has different meaning than author's intention. It's sad it ended up like this because the variable is a useful and convenient idea in general. I agree with your choice (there is no good choice here either way). I've closed this as not planned, add label:wontfix if you wish.

[BurntSushi]

Aye.

For anyone else stumbling across this issue and wondering about a possible path forward, I see two:

1. At some point, people can and should stop caring about legacy Windows console coloring. When that day comes, my sincere hope is that termcolor can be deprecated and we can all move on to using ANSI escape codes as the One True Way to add basic coloring to tty output. And in that world, I think and hope for a much more convenient crate API. When that happens, whoever maintains that new crate can make a different judgment about how to deal with this issue.
2. In theory, I would be okay with changing the NO_COLOR use in this crate to match the new wording of the spec, but it would have to be done in a way that at least surfaces the failure mode to end users. Given that this is a library and not an application, that seems really hard to do. But maybe someone else is more creative than me.