-
Notifications
You must be signed in to change notification settings - Fork 153
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
Add ability to format time and level with user-defined prettifiers #263
Conversation
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.
Could you document this option?
Pull Request Test Coverage Report for Build 1458816767
💛 - Coveralls |
Pull Request Test Coverage Report for Build 1467093039
💛 - Coveralls |
Co-authored-by: James Sumners <james@sumners.email>
Co-authored-by: James Sumners <james@sumners.email>
Co-authored-by: James Sumners <james@sumners.email>
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
Readme.md
Outdated
|
||
Note that prettifiers do not include any coloring, if the stock coloring on `level` is desired, it can be accomplished using the following: | ||
```js | ||
const levelColorize = require('pino-pretty/lib/colors.js')(true) |
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 want to promote relying on an internal API like this. If we are going to support this, we should expose the colorizer as a property of the main export, e.g. const { colorizerFactory } = require('pino-pretty')
.
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.
Good spot, I missed it!
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.
Remove the usage of pino-pretty/lib/colors.js
and expose it.
No problem! |
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.
Last nit, and then everything looks good to me.
Co-authored-by: James Sumners <james@sumners.email>
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.
Looks good to me. Thank you for working with the review process.
@@ -214,4 +214,5 @@ function build (opts = {}) { | |||
|
|||
module.exports = build | |||
module.exports.prettyFactory = prettyFactory | |||
module.exports.colorizerFactory = colors |
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.
Could you add a test to cover this new public API?
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.
Sure - I've expanded the colorizer test to ensure both exports behave the same! If there's something else you had in mind please let me know. 👍
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
This would close issues like #185, and put more formatting capabilities into userland.
If another option-key besides
customPrettifiers
should be used I'm happy to switch. I think the docs are a bit open to interpretation even though I see this option is only being used to prettify object-formatting right now.