Skip to content
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

pass mkdir option on pino/file transport to destination #1223

Merged

Conversation

mojavelinux
Copy link
Contributor

If mkdir option is specified on the pino/file transport, pass it to the pino.destination.

closes #1221

@mojavelinux mojavelinux force-pushed the issue-1221-pino-file-mkdir-option branch from 6c285cc to bc93da6 Compare November 16, 2021 07:58
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add the documentation for this "new" option? Thanks!

@mojavelinux
Copy link
Contributor Author

mojavelinux commented Nov 16, 2021 via email

@mojavelinux mojavelinux force-pushed the issue-1221-pino-file-mkdir-option branch from bc93da6 to 5cd6127 Compare November 17, 2021 20:24
@@ -4,7 +4,9 @@ const pino = require('./pino')
const { once } = require('events')

module.exports = async function (opts = {}) {
const destination = pino.destination({ dest: opts.destination || 1, sync: false })
const destOpts = { dest: opts.destination || 1, sync: false }
if (opts.mkdir) destOpts.mkdir = true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not add ternary condition on destOpts instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to write it any way that's acceptable to the project. Adding an undefined property broke an existing test. I wrote it in such a way that the existing test would pass.

@mojavelinux
Copy link
Contributor Author

I've added a paragraph with an example to the transports.md docs as requested. Let me know if there's anything you'd like me to change.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina merged commit 2987244 into pinojs:master Nov 18, 2021
@mcollina
Copy link
Member

@mojavelinux could you send another PR with the append flag?

@mojavelinux
Copy link
Contributor Author

Yep! On it.

@mojavelinux mojavelinux deleted the issue-1221-pino-file-mkdir-option branch November 18, 2021 09:20
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add mkdir option to pino/file transport
3 participants