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
Remove colored output on windows #864
Conversation
Currently the textformatter on windows outputs ``←[31mERRO←[0m[0000] test windows`` when coloring is not disabled explicitly. However, windows up to windows 8.1 does not support colored output on cmd entirely. Windows 10 added support for it, which is off by default and has to be enabled via registry or environment variable. Therefore i suggest removing colored output on windows entirely to make the output usable again.
text_formatter.go
Outdated
@@ -102,7 +103,7 @@ func (f *TextFormatter) isColored() bool { | |||
} | |||
} | |||
|
|||
return isColored && !f.DisableColors | |||
return isColored && !f.DisableColors && (runtime.GOOS != "windows") |
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.
Hello, I would like that the ForceColor field of the text formatter takes precedence on the environment check.
I expect that then this flag is set to true the user knows what he is doing.
text_formatter_test.go
Outdated
@@ -443,7 +444,11 @@ func TestTextFormatterIsColored(t *testing.T) { | |||
os.Setenv("CLICOLOR_FORCE", val.clicolorForceVal) | |||
} | |||
res := tf.isColored() | |||
assert.Equal(subT, val.expectedResult, res) | |||
if runtime.GOOS == "windows" { |
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 think we need to updated the test for windows environment where isColored always return false except when ForceColor is set to true.
@ceriath thanks for your contribution, I've added a few comments on you change. Let me know what you think. |
@dgsb i agree, i changed it so setting ForceColor or the environment variables takes precedence. With the environment variables users who explicitly turned on colored output have an easy way to also enable it on logrus without recompiling the application |
@ceriath that's exactly what I had in mind. |
Remove colored output on windows
Fixes #862
Currently the textformatter on windows outputs
←[31mERRO←[0m[0000] test windows
when coloring is not disabled explicitly.
However, windows up to windows 8.1 does not support colored output on cmd entirely. Windows 10 added support for it, which is off by default and has to be enabled via registry or environment variable. Therefore i suggest removing colored output on windows entirely to make the output usable again.