Allow logging meta-formatter to set suffix color

[adamslc]

For example, SolarizedLogging.jl needs this behavior to make log messages readable with a solarized color scheme. By default, the suffix is invisible with this color scheme!

[musm]

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

[adamslc]

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).

[oxinabox]

So this one, and #27977
Are closely related.

If we did both,
#27977 would says the default suffix color was to use the Base.stackframe_lineinfo_color())

I actually kinda think we should be abstracting the displaying of lineinfo.
Cos many functions want to display lineinfo

[KristofferC]

Yes, I think integrating this into

julia/base/client.jl

Lines 6 to 31 in a897365

	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]
	function repl_color(key, default)
	env_str = get(ENV, key, "")
	c = tryparse(Int, env_str)
	c_conv = something(c, Symbol(env_str))
	haskey(text_colors, c_conv) ? c_conv : default
	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)]
	stackframe_lineinfo_color() = repl_color("JULIA_STACKFRAME_LINEINFO_COLOR", :bold)
	stackframe_function_color() = repl_color("JULIA_STACKFRAME_FUNCTION_COLOR", :bold)

so users can set a consistent colorscheme is better than to special case the logger.

[adamslc]

Using the base color scheme makes sense. The next question is, should ConsoleLogger continue to use the three-value meta-formatter, or should it use this four-value version?

I'll update this PR to use the base color scheme.

[adamslc]

Bump. It would be awesome if this, or something like this, made it into 1.1. Perhaps @c42f could take a look?

[oxinabox]

it is generally recommended to rebase PR on top of master, rather than merging master into PR.

[KristofferC]

If it needs to be rebased then rebase on the branch you want to merge into.

[adamslc]

Hmmm, I didn't realize that Github's default was to merge master into the branch. Oh well, should be fixed now.

[KristofferC]

It's not a big deal at all though, it will all get squashed away. If the UI is convenient, just use that.

[c42f]

Apologies for the long delay, this slipped my notice when it originally came up.

Overall this looks reasonable but I don't really see the sense in tying anything in logging to the value of stackframe_lineinfo_color. The obvious alternative is to create yet another environment variable for controlling the suffix color, but I'm not super keen on that.

On the other hand, the fact that printstyled("asdf", color=:light_black) is invisible will presumably be a problem for any julia code making use of printstyled with :light_black (this now includes @code_typed inlining info for example). With that in mind, perhaps a much simpler solution to the problem of :light_black in solarized is to simply set

Base.text_colors[:light_black] = Base.text_colors[:normal]

(or whatever you like on the right hand side). Would this work for you?

[c42f]

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.

[oxinabox]

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.

[adamslc]

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.

[adamslc]

I suppose that my real objection to the global variable solution is that there is a mechanism to customize almost all of the colors of a log message, but for some reason, the location information is not customization. The goal with this PR was to resolve that inconsistency in a way that was backwards compatible.

[c42f]

The goal with this PR was to resolve that inconsistency in a way that was backwards compatible

You've done a good job with the backward compatibility, I appreciate it!

@oxinabox I disagree that lineinfo should always be styled the same because the context matters a lot when styling. For example, this is why css selectors allow you to match the inheritance structure of the html DOM: it's not only the object type which matters when styling, but the parent objects which contain it; that is, the context in which it is presented. For a concrete example, consider the styling of the PR title/author on github. The styling is very different when it's presented in the list of all PRs vs when it's presented on the page we're on right now.

On a meta level, there's multiple things that still bother me about the way text is styled in julia, so I don't think anything we do here is a really permanent solution. I think the root of the problem is that we really don't have any equivalent of the html+css separation of content and style, so we're currently limited to having a big pile of environment variables for setting ANSI color. This bothers me, but I'm not sure what the right solution is yet. (Crazy idea: ditch ANSI color entirely for "text/plain" content and replace it with a html+css REPLDisplay for styled text? Then if you want a solarized logger color scheme, just override the css used in the REPLDisplay and be done with it.)

To come back to this issue: if you want this feature, my preference is that we restrict the changes to stdlib/Logging as you have already done, just don't tie it to the styling of line numbers used in backtraces. For consistency I guess we just continue to use the somewhat icky ENV method of setting styles for now and introduce a new environment variable for that. This will have to be deprecated in the future if we get a better system, but so would the other mechanisms for setting color in Base!

[c42f]

Another thought: to make this less breaking in the future if we need to add more style information... it might be better to return a NamedTuple from the default_metafmt?

[c42f]

Regarding color printing, it's also worth reading the discussion leading up to #14052 (comment), and the associated PR at #27430. @vtjnash it's interesting what you're doing there and I think there's a connection to this issue where I've been mulling over how to do color styling of text in a sane way (ie, not adding any more to the pile of environment variables we have right now).

It seems to me that our problem with text styling is that we don't separate content from style very well, and that we need something more similar to html+css in spirit. That is, the style should be controlled by the caller (in the IOContext state if it's done eagerly) and the job of show etc is to only emit the content while respecting that style. Styling of nested content is "interesting", and a full solution would require something like css selectors which can match on the nested tree of content... at which point you arguably might want to give up on plain text and implement a REPLDisplay which processes HTML+css directly.

[vtjnash]

@c42f That sounds like exactly my line of reasoning too! Indeed, part of that work was motived by my desire to use CSS to style the output—and I don't think that IOContext is entirely up to that task. I'm a bit sad that that PR got stopped by triage from going into v1.0, but I'll hopefully get a chance to pick it up again someday (the latest version before work stalled is actually master...vtjnash:jn/iocolor2, since I wanted to really push the limits on it before merging). Anyways, I'd love to hear your thoughts on it more someday.

[c42f]

Indeed, part of that work was motived by my desire to use CSS to style the output—and I don't think that IOContext is entirely up to that task

Very interesting, and agreed about IOContext; I think its job is better suited to limiting output (for performance reasons), leaving styling up to a presentation layer. To not derail the discussion here further, I'll comment over at #27430 if I can make the time to look through that fairly epic diff! Please feel free to ping me if you get to work on this again.

[oxinabox]

@c42f I am convinced by you argument.

[vtjnash]

Is this still in progress? It appears to be stale. I know there was also recent activity trying to configure the colors of stacktraces, and wasn't sure how this fit into that.

[adamslc]

Yes, this has stalled out. It is still an issue, but the issue is larger than just the formatting of log messages as evidenced by the (small) outcry about the new stack-trace printing. A more general solution is probably needed.