make print_with_color emit starting and terminating ANSI sequences on each line #25289

KristofferC · 2017-12-27T23:14:45Z

This is useful if you for example want to print something before each line of colored text.

For example:

function boxify(io, str)
	lines = split(str, '\n')
	for (i, line) in enumerate(lines)
		i == 1 || print(io, '\n')
		if length(lines) == 1
			print(io, "[ ")
		elseif i == 1
			print(io, "┌ ")
		elseif i == length(lines)
			print(io, "└ ")
		else
			print(io, "│ ")
		end
		print(io, line)
	end
end

str = """
This is a string
that has many lines
we will print it
in color"""

io = IOBuffer(); print_with_color(:red, IOContext(io, :color => true), str)
str_colored = String(take!(io));
boxify(STDOUT, str_colored)

Before this PR this will show

now it shows:

KristofferC · 2017-12-28T01:36:19Z

base/util.jl

+        iob = IOBuffer()
+        for line in lines
+            first || print(iob, '\n')
+            first = false


Perhaps check if line is empty here and continue to not write sequences on empty lines.

vtjnash

This does sound like a good idea. It would also help any time formatting gets messed up in the middle of a line, by ensuring that the formatting gets reset for the next line.

vtjnash · 2017-12-28T03:52:12Z

base/util.jl

+            iscolor && print(iob, get(text_colors, color, color_normal))
+            print(iob, line)
+            iscolor && color != :nothing && print(iob, get(disable_text_style, color, text_colors[:default]))
+            iscolor && (bold || color == :bold) && print(iob, disable_text_style[:bold])


it seems silly to copy line to iob one at a time if !iscolor

Updated with

if !iscolor print(io, str) else ...

KristofferC · 2018-01-04T02:03:34Z

Good to go?

stevengj · 2018-01-04T16:28:50Z

base/util.jl

+        else
+            lines = split(str, '\n')
+            first = true
+            iob = IOBuffer()


Can we re-use buf here since it was just emptied?

stevengj · 2018-01-04T16:29:55Z

base/util.jl

+                first || print(iob, '\n')
+                first = false
+                isempty(line) && continue
+                iscolor && bold && print(iob, text_colors[:bold])


iscolor is always true in this loop, no?

Indeed, removed the checks.

stevengj · 2018-01-04T16:32:19Z

base/util.jl

+                iscolor && print(iob, get(text_colors, color, color_normal))
+                print(iob, line)
+                iscolor && color != :nothing && print(iob, get(disable_text_style, color, text_colors[:default]))
+                iscolor && (bold || color == :bold) && print(iob, disable_text_style[:bold])


Won't the color == :bold case be handled by the previous line? It seems like you are printing disable_text_style[:bold] twice for color == :bold.

This is true. Fixed. Also made it not double print when passing both :color = bold and bold = true.

… each line

KristofferC · 2018-01-04T21:10:47Z

Updated from @stevengj's review

stevengj · 2018-01-04T22:44:36Z

base/util.jl

+                first || print(buf, '\n')
+                first = false
+                isempty(line) && continue
+                bold && color != :bold && print(buf, text_colors[:bold])


Seems better to just add a

bold && color == :bold && (color = :nothing)

line above and then you can just use bold && print(...) in the loop.

Alright, did this and shoved the dictionary lookups outside the loop.

stevengj · 2018-01-05T03:40:10Z

base/util.jl

+            enable_ansi  = get(text_colors, color, text_colors[:default]) *
+                               (bold ? text_colors[:bold] : "")
+            disable_ansi = get(disable_text_style, color, text_colors[:default]) *
+                               (bold ? disable_text_style[:bold] : "")


Does it matter if you don't end in the opposite order? i.e. if you do red*bold does it need to end with disablebold*defaultcolor?

It doesn't matter but changed it anyway. It seems nicer.

stevengj · 2018-01-05T03:40:42Z

base/util.jl

+                               (bold ? disable_text_style[:bold] : "")
            lines = split(str, '\n')
            first = true
            for line in lines


for line in split(str, '\n') would make it clearer that the lines variable is not used elsewhere.

c42f · 2018-01-07T11:02:21Z

Oh ho, very nice 👍

Fix for log colorization test due to merge overlap with #25289

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

KristofferC commented Dec 28, 2017

View reviewed changes

KristofferC mentioned this pull request Dec 28, 2017

remove Markdown's own color printing functionality, use Base's instead #25293

Merged

KristofferC force-pushed the kc/reset_newline_colors branch from 9c67cc9 to 4093381 Compare December 28, 2017 02:14

vtjnash reviewed Dec 28, 2017

View reviewed changes

KristofferC force-pushed the kc/reset_newline_colors branch from 4093381 to b7237d4 Compare December 28, 2017 15:54

stevengj reviewed Jan 4, 2018

View reviewed changes

KristofferC added 2 commits January 4, 2018 21:46

make print_with_color emit starting and terminating ANSI sequences on…

3177fde

… each line

improvements from review

47fa29a

KristofferC force-pushed the kc/reset_newline_colors branch from b7237d4 to 47fa29a Compare January 4, 2018 21:09

stevengj reviewed Jan 4, 2018

View reviewed changes

improvements

5c7c2cf

stevengj reviewed Jan 5, 2018

View reviewed changes

small tweaks

9f53aef

stevengj approved these changes Jan 6, 2018

View reviewed changes

StefanKarpinski merged commit 0a98aa9 into master Jan 6, 2018

StefanKarpinski deleted the kc/reset_newline_colors branch January 6, 2018 23:38

c42f mentioned this pull request Jan 7, 2018

ConsoleLogger for more fully featured log printing #25370

Merged

c42f added a commit that referenced this pull request Jan 7, 2018

Fix for log colorization test due to merge overlap with #25289

9bd6fec

c42f mentioned this pull request Jan 7, 2018

Fix for log colorization test due to merge overlap with #25289 #25438

Merged

StefanKarpinski added a commit that referenced this pull request Jan 7, 2018

Merge pull request #25438 from JuliaLang/cjf/fix-log-color-test

7b4ebf4

Fix for log colorization test due to merge overlap with #25289

Uh oh!

make print_with_color emit starting and terminating ANSI sequences on each line #25289

make print_with_color emit starting and terminating ANSI sequences on each line #25289

Uh oh!

Conversation

KristofferC commented Dec 27, 2017

Uh oh!

KristofferC Dec 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vtjnash left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KristofferC commented Jan 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KristofferC commented Jan 4, 2018

Uh oh!

stevengj Jan 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

c42f commented Jan 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

KristofferC Dec 28, 2017 •

edited

Loading

stevengj Jan 4, 2018 •

edited

Loading