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 append option on pino/file transport to pino.destination function #1229

Merged

Conversation

@mojavelinux
Copy link
Contributor

@mojavelinux mojavelinux commented Nov 18, 2021

If append option is specified on the pino/file transport, and the value is false, pass it to the pino.destination function.

closes #1222

Copy link
Member

@mcollina mcollina left a comment

lgtm

@mcollina mcollina merged commit bfce37c into pinojs:master Nov 18, 2021
12 checks passed
@mojavelinux mojavelinux deleted the issue-1222-pino-file-append-option branch Nov 18, 2021
const destOpts = { dest: opts.destination || 1, sync: false }
if (opts.append === false) destOpts.append = false
if (opts.mkdir) destOpts.mkdir = true
const destination = pino.destination(destOpts)
Copy link
Member

@jsumners jsumners Nov 18, 2021

Choose a reason for hiding this comment

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

Would this not be more comprehensive?

const destOpts = Object.assign({}, opts.destination, { dest: opts.destination || 1, sync: false })

Copy link
Contributor Author

@mojavelinux mojavelinux Nov 19, 2021

Choose a reason for hiding this comment

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

That's certainly a valid approach, but it would expand the scope of this issue. If you prefer to forward all the options to the destination, I won't object. @mcollina, where do you stand on this?

Copy link
Member

@mcollina mcollina Nov 19, 2021

Choose a reason for hiding this comment

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

go for it, it's better

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants