Check NO_COLOR flag before CLICOLOR_FORCE#2763
Check NO_COLOR flag before CLICOLOR_FORCE#2763braydenkrus wants to merge 2 commits intoninja-build:masterfrom
NO_COLOR flag before CLICOLOR_FORCE#2763Conversation
In response to Issue ninja-build#2295 (work in progress).
| // Try enabling ANSI escape sequence support on Windows 10 terminals. | ||
| if (supports_color_) { | ||
| char* no_color = getenv("NO_COLOR"); | ||
| if (no_color != NULL && std::string(no_color) != "0") { |
There was a problem hiding this comment.
nit: According to https://bixense.com/clicolors/, the simple fact that NO_COLOR is set should be enough to turn off color output, i.e. no interpretation of the "0" value should be necessary.
| } else { | ||
| const char* clicolor_force = getenv("CLICOLOR_FORCE"); | ||
| supports_color_ = clicolor_force && std::string(clicolor_force) != "0"; | ||
| } |
There was a problem hiding this comment.
nit: It's unfortunate that, even in the original code, CLICOLOR_FORCE is only tested on Unix, and never on Windows. What would you think about using the same logic for both systems, e.g. by moving the detection code to helper functions (e.g. EnvHasNoColor() and EnvHasCliColorForce()) and calling them consistently?
digit-google
left a comment
digit-google
left a comment
There was a problem hiding this comment.
Please squash this into a single commit. It might be useful to add a link to https://bixense.com/clicolors/ in a comment explaining what rules the probes follow. @jhasse, do you think a check for CLICOLOR would be useful here? I see that there is also FORCE_COLOR described at https://force-color.org/
|
Hello @digit-google! I'm on board with your advice. Also, there may be value in switching from |
Good point, this seems important too. @khasse what do you think, since you wrote https://bixense.com/clicolors/ ? |
2 participants
In response to Issue #2295. This is in progress, and I still need to properly test and format these changes.