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

Some logging printing improvements #25111

Merged
merged 1 commit into from
Dec 26, 2017
Merged

Some logging printing improvements #25111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions base/client.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ have_color = false
default_color_warn = :yellow
default_color_error = :light_red
default_color_info = :cyan
default_color_debug = :blue
default_color_input = :normal
default_color_answer = :normal
color_normal = text_colors[:normal]
Expand All @@ -83,6 +84,7 @@ end
error_color() = repl_color("JULIA_ERROR_COLOR", default_color_error)
warn_color() = repl_color("JULIA_WARN_COLOR" , default_color_warn)
info_color() = repl_color("JULIA_INFO_COLOR" , default_color_info)
debug_color() = repl_color("JULIA_DEBUG_COLOR" , default_color_debug)

input_color() = text_colors[repl_color("JULIA_INPUT_COLOR", default_color_input)]
answer_color() = text_colors[repl_color("JULIA_ANSWER_COLOR", default_color_answer)]
Expand Down
35 changes: 21 additions & 14 deletions base/logging.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,23 +483,30 @@ function handle_message(logger::SimpleLogger, level, message, _module, group, id
logger.message_limits[id] = remaining - 1
remaining > 0 || return
end
levelstr = string(level)
Copy link
Member

Choose a reason for hiding this comment

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

Changing this could end up a bit misleading for custom log levels.

Copy link
Member Author

Choose a reason for hiding this comment

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

@c42f would you be OK with renaming Warn to Warning? The reason for this change is that all the other levels are nouns, and I prefer we print Warning over Warn.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's Warn vs Warning which is the sticking point here. The upside of the current naming is that Warn is consistent with @warn, and all the standard levels are four or five characters long.

So it seems impossible to have complete consistency without renaming to @warning in which case I'd rather just leave it as it currently is. Of course, we can just special case the printing more or less as you've done, but taking into account custom levels. I agree it's not entirely satisfying.

Copy link
Member

Choose a reason for hiding this comment

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

Warning seems very standarded to use, looking at random pictures of logging on google... Forcing the levelstr to be the same as the macro used seems a bit inflexible imo. Why can't we use @warn and print it as Warning?

Copy link
Member

@c42f c42f Dec 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Certainly, we can print it differently for logging output, absolutely no problem with that. I thought the issue was about the named constant Logging.Warn::LogLevel.

For achieving "Warning" printed in the output we can have

  1. string(Logging.Warn) == "Warning" - seems too inconsistent to really consider.
  2. Rename Logging.Warn -> Logging.Warning (which I thought was being suggested above). I'm not so keen on that, for the reasons of consistency with the spelling of @warn.
  3. Just special case the output formatting, as is done already. This seems the best of the lot IMO, though not entirely satisfying.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps I was confused about what @fredrikekre was suggesting. Sorry if I was misunderstanding.

Copy link
Member

Choose a reason for hiding this comment

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

No probs, I really wish we could get together in the same room to discuss where this stuff is going. It's super slow and easy to get confused / talk at cross purposes in text.

Option (2) is a reasonable way out if you want to try it @fredrikekre and see how it feels? I'm not really sure where the best compromise of (2) vs (3) lies. They both have some inconsistencies, unfortunately.

color = level < Info ? :blue :
level < Warn ? :cyan :
level < Error ? :yellow : :red
levelstr, color = level < Info ? ("Debug", Base.debug_color()) :
level < Warn ? ("Info", Base.info_color()) :
level < Error ? ("Warning", Base.warn_color()) :
("Error", Base.error_color())
buf = IOBuffer()
iob = IOContext(buf, logger.stream)
print_with_color(color, iob, first(levelstr), "- ", bold=true)
msglines = split(string(message), '\n')
for i in 1:length(msglines)-1
println(iob, msglines[i])
print_with_color(color, iob, "| ", bold=true)
end
println(iob, msglines[end], " -", levelstr, ":", _module, ":", basename(filepath), ":", line)
for (key,val) in pairs(kwargs)
print_with_color(color, iob, "| ", bold=true)
println(iob, key, " = ", val)
msglines = split(chomp(string(message)), '\n')
if length(msglines) + length(kwargs) == 1
print_with_color(color, iob, "[ ", levelstr, ": ", bold=true)
print(iob, msglines[1], " ")
else
print_with_color(color, iob, "┌ ", levelstr, ": ", bold=true)
println(iob, msglines[1])
for i in 2:length(msglines)
print_with_color(color, iob, "│ ", bold=true)
println(iob, msglines[i])
end
for (key,val) in pairs(kwargs)
print_with_color(color, iob, "│ ", bold=true)
println(iob, " ", key, " = ", val)
Copy link
Member

Choose a reason for hiding this comment

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

In particular - if you want exceptions to be readable, it's the printing of val on this line here which should be beefed up.

end
print_with_color(color, iob, "└ ", bold=true)
end
print_with_color(:light_black, iob, "@ ", _module, " ", basename(filepath), ":", line, "\n")
write(logger.stream, take!(buf))
nothing
end
Expand Down
14 changes: 8 additions & 6 deletions test/logging.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,22 +249,24 @@ end
# Simple
@test genmsg(Info, "msg", Main, "some/path.jl", 101) ==
"""
I- msg -Info:Main:path.jl:101
[ Info: msg @ Main path.jl:101
"""

# Multiline message
@test genmsg(Warn, "line1\nline2", Main, "some/path.jl", 101) ==
"""
W- line1
| line2 -Warn:Main:path.jl:101
┌ Warning: line1
│ line2
└ @ Main path.jl:101
"""

# Keywords
@test genmsg(Error, "msg", Base, "other.jl", 101, a=1, b="asdf") ==
"""
E- msg -Error:Base:other.jl:101
| a = 1
| b = asdf
┌ Error: msg
│ a = 1
│ b = asdf
└ @ Base other.jl:101
"""
end

Expand Down