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 prettyPrint to pino constructor options #186
Conversation
docs/API.md
Outdated
@@ -53,6 +53,8 @@ | |||
Default: 'info'. | |||
* `levelVal` (integer): when defining a custom log level via `level`, set to an | |||
integer value to define the new level. Default: `undefined`. | |||
* `prettyPrint` (boolean|object): enables [pino.pretty](#pretty). This is intended for non-production | |||
configurations. This may be set to a configuration object as outlinedin [pino.pretty](#pretty). Default: `false`. |
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.
small thing:
+ outlinedin
- outlined in
@@ -83,6 +85,8 @@ Returns a new [logger](#logger) instance. | |||
timestamp to ISO 8601 date format, and reserialize the JSON (equivalent to `pino -t`). | |||
* `formatter` (function): a custom function to format the line, is passed the | |||
JSON object as an argument and should return a string value. | |||
* `levelFirst` (boolean): if set to `true`, it will print the name of the log | |||
level as the first field in the log line. Default: `false`. |
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 may be missing something obvious, but I don't see this referenced in the code.
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.
Ah, I see – was this just missing from the docs before?
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.
Yes, it was just missing from the docs. Noticed it while adding the feature.
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, just a nit.
pino.js
Outdated
if (iopts.prettyPrint) { | ||
var pstream = pretty(iopts.prettyPrint) | ||
var origStream = istream | ||
pstream.pipe(origStream) |
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 think we should use pump
.
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.
Done. But I left off an error function because we are not within an actual instance at that point. Thus, we can't emit the error.
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 can because the logger will be there if an error is ever emitted. It's never emitted synchronously during pipe
with streams.
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.
Okay, I added it. Personally, I don't like it because it relies on hoisting.
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, feel free to merge and release! 🎉
note - unrelated failing test in Node 7 |
I bet that error has to do with issue #131. |
@jsumners you bet correctly |
🎉 |
1 similar comment
🎉 |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
See the discussion in issue #184 for the motivation to this PR.