with_output_color writes control codes to non-terminal streams

[stevengj]

This doesn't seem right:

julia> buf = IOBuffer(); warn(buf, "foo"); String(take!(buf))
"\e[1m\e[33mWARNING: \e[39m\e[22m\e[33mfoo\e[39m\n"

This seems to be the behavior in 0.4 and 0.5 as well. However, something related to this has recently started breaking this test in Compat.jl. The most recent change to the relevant file base/util.jl seems to be #18628; @KristofferC, could that have affected the Compat test?

[KristofferC]

The problem here is that before that PR the output was something like:

\e[1m\e[33mWARNING: foo\e[0m\n

Previously, the whole string "WARNING: foo" was in one "piece" with no ANSI codes in between and the contains call thus passed. Now, the WARNING part is in bold but the rest isn't, so there is a terminating ANSI code after the "WARNING" part.

The only thing that determines or not whether Julia should use colors is the global variable have_color, i.e. it is independent of the stream.

[stevengj]

Yeah, I just realized that Jupyter supports ANSI color codes even though it is not a TTY. Still, I wonder if this should be some kind of IOContext thing.

[KristofferC]

Yeah, it most likely should. It has also been discussed in some places. Maybe most of it could be solved by moving to an IOContext with a :hascolor key and just let STDOUT and STDERR be IOContexts with that field set to true/false in the same way that Base.have_color is now determined.

[stevengj]

You could also just define getindex(::TTY, key) = key == :hascolor ? have_color : throw(KeyError()) to avoid changing STDOUT and STDERR.

[StefanKarpinski]

Can this be closed now that JuliaLang/Compat.jl#293 has been merged or is there still something to be done in Base Julia?

[stevengj]

There is the still question of whether have_color should be an IO attribute rather than a global.

[StefanKarpinski]

This strikes me as a bugfix that would be eligible for fixing at any time, but we may as well fix it. We need IOContext(io, :color => [true|false] to fix this correctly.

[JeffBezanson]

And ideally remove the have_color global.

[JeffBezanson]

We can add a get method to TTY that returns true or false for :color based on the detection we do at startup.