Skip to content
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.

Sign up for GitHub

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

Allow logging meta-formatter to set suffix color #29397

Closed
wants to merge 4 commits into from
Closed

Allow logging meta-formatter to set suffix color #29397

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
abfe771
allow metaformatter to set suffix color
Sep 27, 2018
4e378ba
update docs
Sep 27, 2018
fdf02f5
make changes backwards compatiable
adamslc Oct 2, 2018
ebbc799
Make ConsoleLogger colors consistant with Base
adamslc Oct 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 18 additions & 10 deletions stdlib/Logging/src/ConsoleLogger.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@ Log levels less than `min_level` are filtered out.
Message formatting can be controlled by setting keyword arguments:

* `meta_formatter` is a function which takes the log event metadata
`(level, _module, group, id, file, line)` and returns a color (as would be
passed to printstyled), prefix and suffix for the log message. The
default is to prefix with the log level and a suffix containing the module,
`(level, _module, group, id, file, line)` and returns a prefix color (as would
be passed to printstyled), prefix, suffix color, and suffix for the log message.
The default is to prefix with the log level and a suffix containing the module,
file and line location.
* `show_limited` limits the printing of large data structures to something
which can fit on the screen by setting the `:limit` `IOContext` key during
Expand Down Expand Up @@ -58,10 +58,11 @@ function default_logcolor(level)
end

function default_metafmt(level, _module, group, id, file, line)
color = default_logcolor(level)
prefix_color = default_logcolor(level)
prefix = (level == Warn ? "Warning" : string(level))*':'
suffix = ""
Info <= level < Warn && return color, prefix, suffix
suffix_color = Base.stackframe_lineinfo_color()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea with light_black was to not distract people too much with the logging metadata and I'd rather like to preserve that. But stackframe_lineinfo_color is :bold which is the opposite.

I'm also not sure why we'd choose stackframe_lineinfo_color here, as it's not clearly related to logging. We could just make a new color category and matching environment variable and set it default to :light_black.

Copy link
Contributor

@oxinabox oxinabox Nov 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was on the opinion that they were the same notional thing, both are line info.
My proposal was more generally that we should have a single print_styled_lineinfo, used by all 3 of logging and methods and error printing.

To my mind the line number shown in the log is a mini-stack-trace.
Like a @warn is a softer alternative to an exception.

But I do see your point. About at one point it being considered important and the other not.
However, it is also worth noting that on many (most?) terminals :bold white is more or less indistinguishable from :white (I actually find that it is harder find the line info section in the stack trace where it is :bold white than in the warning info where is is :light_black)
Having 2 variables for more configurability is not unreasonable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I originally set the default suffix_color to :light_black, which I thought was preferable. But other people wanted to emphasize the similarities between the mini stacktrace and a full stacktrace.

Info <= level < Warn && return prefix_color, prefix, suffix_color, suffix
_module !== nothing && (suffix *= "$(_module)")
if file !== nothing
_module !== nothing && (suffix *= " ")
Expand All @@ -71,7 +72,7 @@ function default_metafmt(level, _module, group, id, file, line)
end
end
!isempty(suffix) && (suffix = "@ " * suffix)
return color, prefix, suffix
return prefix_color, prefix, suffix_color, suffix
end

# Length of a string as it will appear in the terminal (after ANSI color codes
Expand Down Expand Up @@ -127,7 +128,14 @@ function handle_message(logger::ConsoleLogger, level, message, _module, group, i

# Format lines as text with appropriate indentation and with a box
# decoration on the left.
color,prefix,suffix = logger.meta_formatter(level, _module, group, id, filepath, line)
format = logger.meta_formatter(level, _module, group, id, filepath, line)
if length(format) == 4
prefix_color, prefix, suffix_color, suffix = format
else
# The three-value version of meta_formatter should be depreciated at some point
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK this is an unexported API function and thus this change can go into Julia 1.1 or the next minor bump of the Logging package.

This should also have an explicit deprecation message for the 3 value version, so developers can plan for this change appropriately

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends how you define public API. The docstring for ConsoleLogger mentions the meta_formatter option, which I would argue makes it part of the public API (even if the default meta formatter is unexported).

prefix_color, prefix, suffix = format
suffix_color = :light_black
end
minsuffixpad = 2
buf = IOBuffer()
iob = IOContext(buf, logger.stream)
Expand All @@ -145,15 +153,15 @@ function handle_message(logger::ConsoleLogger, level, message, _module, group, i
i == 1 ? "┌ " :
i < length(msglines) ? "│ " :
"└ "
printstyled(iob, boxstr, bold=true, color=color)
printstyled(iob, boxstr, bold=true, color=prefix_color)
if i == 1 && !isempty(prefix)
printstyled(iob, prefix, " ", bold=true, color=color)
printstyled(iob, prefix, " ", bold=true, color=prefix_color)
end
print(iob, " "^indent, msg)
if i == length(msglines) && !isempty(suffix)
npad = max(0, justify_width - nonpadwidth) + minsuffixpad
print(iob, " "^npad)
printstyled(iob, suffix, color=:light_black)
printstyled(iob, suffix, color=suffix_color)
end
println(iob)
end
Expand Down
27 changes: 14 additions & 13 deletions stdlib/Logging/test/runtests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,30 +41,31 @@ import Logging: min_enabled_level, shouldlog, handle_message
end
@test String(take!(buf)) == ""

suffix_color = Base.stackframe_lineinfo_color()
@testset "Default metadata formatting" begin
@test Logging.default_metafmt(Logging.Debug, Base, :g, :i, expanduser("~/somefile.jl"), 42) ==
(:blue, "Debug:", "@ Base ~/somefile.jl:42")
(:blue, "Debug:", suffix_color, "@ Base ~/somefile.jl:42")
@test Logging.default_metafmt(Logging.Info, Main, :g, :i, "a.jl", 1) ==
(:cyan, "Info:", "")
(:cyan, "Info:", suffix_color, "")
@test Logging.default_metafmt(Logging.Warn, Main, :g, :i, "b.jl", 2) ==
(:yellow, "Warning:", "@ Main b.jl:2")
(:yellow, "Warning:", suffix_color, "@ Main b.jl:2")
@test Logging.default_metafmt(Logging.Error, Main, :g, :i, "", 0) ==
(:light_red, "Error:", "@ Main :0")
(:light_red, "Error:", suffix_color, "@ Main :0")
# formatting of nothing
@test Logging.default_metafmt(Logging.Warn, nothing, :g, :i, "b.jl", 2) ==
(:yellow, "Warning:", "@ b.jl:2")
(:yellow, "Warning:", suffix_color, "@ b.jl:2")
@test Logging.default_metafmt(Logging.Warn, Main, :g, :i, nothing, 2) ==
(:yellow, "Warning:", "@ Main")
(:yellow, "Warning:", suffix_color, "@ Main")
@test Logging.default_metafmt(Logging.Warn, Main, :g, :i, "b.jl", nothing) ==
(:yellow, "Warning:", "@ Main b.jl")
(:yellow, "Warning:", suffix_color, "@ Main b.jl")
@test Logging.default_metafmt(Logging.Warn, nothing, :g, :i, nothing, 2) ==
(:yellow, "Warning:", "")
(:yellow, "Warning:", suffix_color, "")
@test Logging.default_metafmt(Logging.Warn, Main, :g, :i, "b.jl", 2:5) ==
(:yellow, "Warning:", "@ Main b.jl:2-5")
(:yellow, "Warning:", suffix_color, "@ Main b.jl:2-5")
end

function dummy_metafmt(level, _module, group, id, file, line)
:cyan,"PREFIX","SUFFIX"
:cyan,"PREFIX",:light_black,"SUFFIX"
end

# Log formatting
Expand Down Expand Up @@ -98,7 +99,7 @@ import Logging: min_enabled_level, shouldlog, handle_message
# Full metadata formatting
@test genmsg("msg", level=Logging.Debug,
meta_formatter=(level, _module, group, id, file, line)->
(:white,"Foo!", "$level $_module $group $id $file $line")) ==
(:white,"Foo!",:red, "$level $_module $group $id $file $line")) ==
"""
┌ Foo! msg
└ Debug Main a_group an_id some/path.jl 101
Expand All @@ -116,11 +117,11 @@ import Logging: min_enabled_level, shouldlog, handle_message
└ SUFFIX
"""
# Behavior with empty prefix / suffix
@test genmsg("msg", meta_formatter=(args...)->(:white, "PREFIX", "")) ==
@test genmsg("msg", meta_formatter=(args...)->(:white, "PREFIX", :red, "")) ==
"""
[ PREFIX msg
"""
@test genmsg("msg", meta_formatter=(args...)->(:white, "", "SUFFIX")) ==
@test genmsg("msg", meta_formatter=(args...)->(:white, "", :red, "SUFFIX")) ==
"""
┌ msg
└ SUFFIX
Expand Down