-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Added support to disable output coloring from env variables #6078
Added support to disable output coloring from env variables #6078
Conversation
I don't think this is right. Detecting color support is a lot more complicated than that. We could consider using https://github.com/jwalton/go-supportscolor which is a pretty simple library which has robust detection heuristics. WDYT @mholt ? |
Sure, although that package doesn't seem to support What if we start with this, then change to using that package if we get more requests/complaints? I like that this is simple, conventional, and makes it really clear/obvious (to us) when color is used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NO_COLOR can have any value, I think -- other than that, we can try this out I think, for starters!
logging.go
Outdated
@@ -758,6 +761,7 @@ func Log() *zap.Logger { | |||
} | |||
|
|||
var ( | |||
coloringEnabled = os.Getenv("NO_COLOR") != "1" && os.Getenv("TERM") != "xterm-mono" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
coloringEnabled = os.Getenv("NO_COLOR") != "1" && os.Getenv("TERM") != "xterm-mono" | |
coloringEnabled = os.Getenv("NO_COLOR") != "" && os.Getenv("TERM") != "xterm-mono" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mholt The example implementation on the no-color.org website linked in the issue shows that if the NO_COLOR
is set to "0", we should set the coloring to false. I updated the code to follow this implementation. Let me know what you think about it and I can update again accordingly.
int
main(int argc, char *argv[])
{
char *no_color = getenv("NO_COLOR");
bool color = true;
if (no_color != NULL && no_color[0] != '\0')
color = false;
/* do getopt(3) and/or config-file parsing to possibly turn color back on */
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that's actually the null byte, if my C memory isn't underflowing me.
The actual verbiage says:
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.
So presumably a value of 0 still counts as "set", but... that is kinda weird to do. I dunno why set the variable at all in that case. 🤷♂️ Maybe we just leave it as-is for now and see if anyone complains (with good reason for setting it to 0)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I believe you're right, I did not see the text with it. My C knowledge is also very rusty at this point so I missed that. I will update it to conform to the specs.
The only time color should be enabled is when we use stdout or stderr as the output module. In all other cases, it doesn't make sense. We would never enable it for files. The lib is able to look at the stout/stderr handles to see if they support color. |
631391e
to
c61693f
Compare
c61693f
to
ce0f061
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say this looks good to me now -- we can try it! Thank you @armadi1809
Closes #6077