Implement the standard logging levels with same logic as custom log levels #52696
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
oxinabox
commented
Jan 2, 2024
Comment on lines
60
to
+70
| function default_logcolor(level::LogLevel) | ||
| level in keys(custom_log_levels) ? custom_log_levels[level][2] : | ||
| level < Info ? Base.debug_color() : | ||
| level < Warn ? Base.info_color() : | ||
| level < Error ? Base.warn_color() : | ||
| Base.error_color() | ||
| color = get(_log_levels, level, nothing) | ||
| if isnothing(color) | ||
| # default colors: based such that colors for standard log levels (Info etc) are in middle of color range | ||
| level = level.level | ||
| color = level < -500 ? Base.debug_color() : | ||
| level < 500 ? Base.info_color() : | ||
| level < 1500 ? Base.warn_color() : | ||
| Base.error_color() | ||
| end | ||
| return color |
There was a problem hiding this comment.
This logic is here, which I am unsure about.
It could instead be inside the code for @create_log_macro
to be called at definition time.
Pros of in putting it in @create_log_macro:
- is that the color wouldn't be Logger specific, so user configured loggers could take advantage of it.
Cons of in putting it in @create_log_macro:
- We would need some trick to make it actually continue to read the enviroment variable behimds
debug_coloretc at runtime (rather than at compile time). I am not sure what that trick would be. - Wouldn't have a definition for what color to use if the log level was defined via the constructor only, rather than the macro. Which people do do right now. Could have the logic in both, or simpler logic that just says use color
1if we do not have a color defined for that level.
|
@IanButterworth thoughts? @quinnj @nickrobinson251 this would make the original design of e.g. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow up to
#52359
Not entirely convinced this is a good idea, but wanted to put it out there for comment.
A big reason why i don't know that it is a good idea is that we can't actually use the custom log level macro as it is in the logging stdlib rather than in
Base.CoreLogging.I thus reimplement the code it would generate, though i could instead move it into
Base.CoreLoggingwhich would probably be cleaner.I do think it is a good feature to have the color be optional and to default to the ones defined by the nearest of error/warn/debug/info.
Since those are user configurable colors.
Which honestly kinda seem like the right colors anyway.
A user defined
traceshould use same color asdebug, and a user definedcriticalshould use the same color as eitherwarnorerrordepending on which it is closer to.Not some made up color that might not match how user has configured their system.