add :color attribute to IOContext, prefer get(io,:color,false) to Base.have_color #25067

stevengj · 2017-12-13T22:45:49Z

Fixes #19682 by making the use of ANSI color/formatting codes specific to the io device. Instead of accessing the Base.have_color global, you should call ~~Base.hascolor(io) (not exported)~~ get(io, :color, false). By default this uses Base.have_color for TTY devices, but can be overridden by the :color => {true,false} property of IOContext.

Also fixes a problem noted on discourse where Markdown was overriding display to do terminal output. Now it overrides show and uses hascolor(io) to decide in a device-specific way whether to output formatting codes.

KristofferC · 2017-12-13T22:48:37Z

I wonder if it should be possible to distinguish between supporting 16, 256, 24bit colors. Just a random thought, just ignore if this is off topic.

vtjnash · 2017-12-13T23:07:48Z

test/replutil.jl

 @test length(methods(method_c1)) <= 3 # because of '...' in candidate printing
-test_have_color(buf,
-                "\e[0m\nClosest candidates are:\n  method_c1(\e[1m\e[31m::Float64\e[0m, \e[1m\e[31m::AbstractString...\e[0m)$cfile$c1line\e[0m",


might be nice to keep (some of?) these, now that we can actually test both code path properly:

Base.show_method_candidates(IOContext(buf, :color => true), Base.MethodError(method_c1,(1, 1, ""))) @test String(take!(buf)) == "\e[0m\nClosest candidates are:\n method_c1(\e[1m\e[31m::Float64\e[0m, \e[1m\e[31m::AbstractString...\e[0m)$cfile$c1line\e[0m"

I think color codes in warnings are tested elsewhere....

But sure, I'll add a few back in with color enabled.

Interestingly enough, the have_color testcase was totally wrong, since at least Julia 0.6. It seems that this hasn't been tested in a long while.

I've added a test.

Proving once again that bitrot is inevitable for code that doesn't actually run!

vtjnash · 2017-12-13T23:08:22Z

base/show.jl

+
+Return `true` if `io` supports/expects ANSI color codes.
+
+(This is determined when Julia is started for terminals, and can be


parens don't seem necessary to me.

vtjnash · 2017-12-13T23:12:10Z

base/show.jl

@@ -67,6 +67,9 @@ The following properties are in common use:
   can be avoided (e.g. `[Float16(0)]` can be shown as "Float16[0.0]" instead
   of "Float16[Float16(0.0)]" : while displaying the elements of the array, the `:typeinfo`
   property will be set to `Float16`).
+ - `:color`: Boolean specifying whether ANSI color/escape codes are supported/expected.


Longer term, I think we may want to try to distinguish between policy ("wants color") and mechanism ("ansi", "xterm256", "html"). But this PR is already strictly better than what we have now, so I think we should merge this and then consider iterating (as needed).

We can add new IO context attributes that Base understand in the future as long as the behavior of the existing ones remains the same.

stevengj · 2017-12-14T20:21:21Z

Note that with this PR, a Markdown object rendered to text/plain is now the "rendered" text (with or without color), since this is how it is displayed (rather than overriding display as was done previously, contrary to usual practice). Previously, text/plain output was identical to text/markdown output, erasing a useful distinction — now, if you want text/markdown output you should show to text/markdown.

(In general, there is a lot of stuff in the documentation system that hooks into the display system a little oddly, e.g. using side-effects … this required some crude hacks in IJulia to get a "clean" docstring out of a help request.)

StefanKarpinski · 2017-12-15T02:33:36Z

Error in docs:

Error in testset docs:
Test Failed at /tmp/julia/share/julia/test/docs.jl:29
  Expression: stringmime("text/plain", Docs.doc(C74685)) == "Doc abstract type\n"
   Evaluated: "  Doc abstract type\n" == "Doc abstract type\n"
Error in testset compile:
Error During Test at nothing:1
  Test threw an exception of type Test.TestSetException
  Expression: compile
  Some tests did not pass: 90 passed, 2 failed, 0 errored, 0 broken.
ERROR: LoadError: Test run finished with errors
in expression starting at /tmp/julia/share/julia/test/runtests.jl:46

stevengj · 2017-12-15T12:06:41Z

Yes, the text/plain output has changed. If you just do sprint or string you will still get the old output, though. I'll fix the test.

stevengj · 2017-12-15T21:15:59Z

Looks like it needs a rebase after #24913 to fix the new deprecation.

stevengj · 2017-12-16T00:21:01Z

I'm getting

Error in testset misc:
Error During Test at /tmp/julia/share/julia/test/testdefs.jl:19
  Got an exception of type LoadError outside of a @test
  LoadError: `warn()` is deprecated, use `@warn` instead.

I guess someone flipped a switch to turn deprecations into errors so that now everything is breaking? (I'm confused.)

StefanKarpinski · 2017-12-16T03:05:08Z

We now had standard logging infrastructure in Base: #24490. Among other changes, warn("message") should now be written as @warn "message".

stevengj · 2017-12-16T14:06:38Z

I'm confused, because tests/misc.jl seems to contain a ton of calls to warn(...) besides the one I added in this PR. @c42f, shouldn't those calls have been updated in #24490?

c42f · 2017-12-16T21:38:56Z

shouldn't those calls have been updated

These test deprecated functionality - the new system is tested separately in test/logging.jl. I could have just removed the old tests, but that seemed premature because deprecated functionality should continue to work until it's removed.

stevengj · 2017-12-16T23:03:08Z

Oh, now I see the if Base.JLOptions().depwarn != 2 at the top...

c42f · 2017-12-17T00:08:30Z

Yeah it's a bit nasty I'm afraid - this is why I want to remove deperror - if it's enabled it's all or nothing, there's really no nice way to test deprecated functionality.

StefanKarpinski · 2017-12-17T01:12:59Z

I would just delete the deprecated functionality tests. After all, we’re going to delete the in 1.0 anyway.

…ing the Base.have_color global

…ut color), distinct from text/markdown

To capture messages from Base, users should install a logger instead, for example, using `with_logger()`.

stevengj · 2017-12-18T14:37:51Z

ci/circleci seems to be an unrelated libgit2 "Error deserializing a remote exception" distributed-test failure.

stevengj added the display and printing Aesthetics and correctness of printed representations of objects. label Dec 13, 2017

vtjnash reviewed Dec 13, 2017

View reviewed changes

stevengj changed the title ~~add :color attribute to IOContext, prefer Base.hascolor(io) to Base.have_color~~ add :color attribute to IOContext, prefer get(io,:color,false) to Base.have_color Dec 14, 2017

StefanKarpinski approved these changes Dec 14, 2017

View reviewed changes

stevengj force-pushed the colorattr branch from e7549cb to 9e0c1ad Compare December 15, 2017 21:20

stevengj and others added 11 commits December 17, 2017 15:43

add :color attribute to IOContext, prefer Base.hascolor(io) to access…

4b04cfe

…ing the Base.have_color global

just use get, not hascolor

b29d3f6

markdown fixes

f54406c

more bugfixes

dc5cf65

added color test

4579b37

bugfix

2b4369b

test for 19682

4c9dd70

test markdown color output

9cd4dc4

NEWS

14eb83e

text/plain markdown output should be the rendered text (with or witho…

e4256fc

…ut color), distinct from text/markdown

fixed test

9a0edbe

stevengj added 3 commits December 17, 2017 15:44

update for JuliaLang#24913

6ed9493

rm test of deprecated warn function

7407ccb

preserve io context for logging stream

0ef0fde

stevengj referenced this pull request Dec 18, 2017

Deprecate old logging info(), warn(), logging()

5d77d1b

To capture messages from Base, users should install a logger instead, for example, using `with_logger()`.

stevengj force-pushed the colorattr branch from 9e0c1ad to 0ef0fde Compare December 18, 2017 02:13

fix test for new text/plain output

36b5308

StefanKarpinski merged commit 88cdf44 into JuliaLang:master Dec 18, 2017

stevengj deleted the colorattr branch December 18, 2017 15:46

This was referenced Dec 21, 2017

fix color attribute in TTY and TTYTerminal to be more IOContext-like #25228

Merged

rm tests of deprecated logging functions #25229

Closed

fredrikekre mentioned this pull request Feb 23, 2018

Bring back color printing JuliaDocs/Documenter.jl#647

Merged

fredrikekre mentioned this pull request May 15, 2018

extra newline displaying markdown objects in REPL #27102

Closed

ararslan mentioned this pull request May 19, 2018

Updates for Julia 0.7 ssfrr/DeepDiffs.jl#9

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add :color attribute to IOContext, prefer get(io,:color,false) to Base.have_color #25067

add :color attribute to IOContext, prefer get(io,:color,false) to Base.have_color #25067

stevengj commented Dec 13, 2017 •

edited

Loading

KristofferC commented Dec 13, 2017

vtjnash Dec 13, 2017

stevengj Dec 13, 2017

stevengj Dec 13, 2017

stevengj Dec 14, 2017

stevengj Dec 14, 2017

StefanKarpinski Dec 14, 2017

vtjnash Dec 13, 2017

vtjnash Dec 13, 2017

StefanKarpinski Dec 14, 2017

stevengj commented Dec 14, 2017

StefanKarpinski commented Dec 15, 2017

stevengj commented Dec 15, 2017 •

edited

Loading

stevengj commented Dec 15, 2017

stevengj commented Dec 16, 2017 •

edited

Loading

StefanKarpinski commented Dec 16, 2017

stevengj commented Dec 16, 2017 •

edited

Loading

c42f commented Dec 16, 2017

stevengj commented Dec 16, 2017

c42f commented Dec 17, 2017

StefanKarpinski commented Dec 17, 2017

stevengj commented Dec 18, 2017


		Return `true` if `io` supports/expects ANSI color codes.

		(This is determined when Julia is started for terminals, and can be

add :color attribute to IOContext, prefer get(io,:color,false) to Base.have_color #25067

add :color attribute to IOContext, prefer get(io,:color,false) to Base.have_color #25067

Conversation

stevengj commented Dec 13, 2017 • edited Loading

KristofferC commented Dec 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengj commented Dec 14, 2017

StefanKarpinski commented Dec 15, 2017

stevengj commented Dec 15, 2017 • edited Loading

stevengj commented Dec 15, 2017

stevengj commented Dec 16, 2017 • edited Loading

StefanKarpinski commented Dec 16, 2017

stevengj commented Dec 16, 2017 • edited Loading

c42f commented Dec 16, 2017

stevengj commented Dec 16, 2017

c42f commented Dec 17, 2017

StefanKarpinski commented Dec 17, 2017

stevengj commented Dec 18, 2017

stevengj commented Dec 13, 2017 •

edited

Loading

stevengj commented Dec 15, 2017 •

edited

Loading

stevengj commented Dec 16, 2017 •

edited

Loading

stevengj commented Dec 16, 2017 •

edited

Loading