-
Notifications
You must be signed in to change notification settings - Fork 166
Feature/colorize message #611
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.
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
base: master
Are you sure you want to change the base?
Conversation
| customColoredColorizer.message = plain.message | ||
| customColoredColorizer.greyMessage = plain.greyMessage | ||
| customColoredColorizer.property = plain.property | ||
| customColoredColorizer.levelColors = plain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is .levelColors being set to the hash of colors on all three colorizers? The other properties, e.g. .message, are set to the color that field will be colorized to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To preserve all color levels and make use of them, .message will remain cyan during tests.
The colorizer will initially select one of the two constants:
colored: message as cyan
or
plain: nocolor;
e.g:
const colored = {
default: white,
60: bgRed,
50: red,
40: yellow,
30: green,
20: blue,
10: gray,
message: cyan,
greyMessage: gray,
property: magenta
}
| if (colorizeMessage) { | ||
| return colorizer.levelColors[getPropertyValue(log, levelKey)](message) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have colorizer.message() to apply color to the message. Shouldn't that method be updated instead of adding this block multiple times?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Complementing the comment in lib/color.js, I thought it would be useful to expose the colors object to facilitate other implementations.
Also, using .message alone didn’t achieve the desired result in my tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand. The sole purpose of colorizer.message is to apply the configured color to the message string. If that is not happening, then something is either wrong with that method or this PR is incomplete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It actually does, but only in Cyan (see #430 print).
I understand your point that this might seem contrary to the library’s design. However, the colorizer.message method ultimately delegates to a function from colorette.
In colors.js:
const { createColors } = require('colorette')
const availableColors = createColors({ useColor: true })
const { ..., cyan, ... } = availableColors
const colored = {
default: white,
60: bgRed,
....
message: cyan,
...
}
function coloredColorizer (useOnlyCustomProps) {
const newColoredColorizer = colorizeLevel(useOnlyCustomProps)
const customColoredColorizer = function (level, opts) {
return newColoredColorizer(level, colored, opts)
}
customColoredColorizer.message = colored.message
...
}Changing colorette itself wouldn’t make sense, since the library is correctly fulfilling its purpose.
The behavior we want to adjust lies within pino-pretty. Modifying the existing factory methods to achieve this would require a broader refactor—essentially introducing middleware logic before calling colorette, which would affect the entire colorization flow.
This PR, instead, introduces an way to colorize the message using the same color as the log level, without overriding the default behavior. It preserves the current integration between pino-pretty and colorette, while giving users more flexibility in how messages are colorized—avoiding design breaks rather than causing them.
|
Could you please check this again, @jsumners ? |
mcollina
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
jsumners
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR seems to be introducing a second means of colorizing the message instead of updating the method designed for doing so to apply a configured color. This is contrary to the overall design of colorizing items across the module.
| if (colorizeMessage) { | ||
| return colorizer.levelColors[getPropertyValue(log, levelKey)](message) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand. The sole purpose of colorizer.message is to apply the configured color to the message string. If that is not happening, then something is either wrong with that method or this PR is incomplete.
Hi!
This PR introduces support for colorful messages in the terminal. It doesn't affect object output, but it can help improve readability when logging to the console.
It also addresses and closes issues #430 and #524.
Example:
