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 merging object to mixins #926

Merged
merged 4 commits into from
Nov 15, 2020
Merged

Conversation

madelinecameron
Copy link
Contributor

@madelinecameron madelinecameron commented Nov 13, 2020

Allows for mixins to take in an object and add information contextually.

For instance, if your mergingObject has an error key (a JS Error), I want to add { message: error.message, stack: error.stack }. This will make mixins more useful or having to pull from higher in the scope.

@@ -147,9 +147,9 @@ function write (_obj, msg, num) {
var obj

if (_obj === undefined || _obj === null) {
obj = mixin ? mixin() : {}
obj = mixin ? mixin({}) : {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not totally needed, just figured I'd be nice and pass an empty object. 🤷

@madelinecameron
Copy link
Contributor Author

Will add a test shortly, sorry about that. :)

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.

Can you please add a unit test?

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
Copy link
Member

This is also missing some docs update

@madelinecameron
Copy link
Contributor Author

Yep, was wondering about that, I'll do that. Thanks!

@madelinecameron
Copy link
Contributor Author

Took a crack at integrating it to documentation, let me know any feedback. :)

@@ -124,7 +124,7 @@ const mixin = {
}

const logger = pino({
mixin() {
mixin({ description }) {
Copy link
Member

Choose a reason for hiding this comment

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

let's name the paramenter obj, without destructuring

@mcollina mcollina merged commit 6c42f14 into pinojs:master Nov 15, 2020
@madelinecameron madelinecameron deleted the contextual-mixin branch November 16, 2020 16:39
hraban added a commit to hraban/pino that referenced this pull request Jan 15, 2021
Restores the original semantics introduced by afb639c (pinojs#857).

Reverts the changes to this documentation from 6e50d72 (pinojs#928) and
6c42f14 (pinojs#926).
@hraban hraban mentioned this pull request Jan 15, 2021
hraban added a commit to hraban/pino that referenced this pull request Jan 15, 2021
Restores the original semantics introduced by afb639c (pinojs#857).

Reverts the changes to this documentation from 6e50d72 (pinojs#928) and
6c42f14 (pinojs#926).

Fixes pinojs#951.
mcollina pushed a commit that referenced this pull request Jan 15, 2021
Restores the original semantics introduced by afb639c (#857).

Reverts the changes to this documentation from 6e50d72 (#928) and
6c42f14 (#926).

Fixes #951.
@github-actions
Copy link

github-actions bot commented Feb 2, 2022

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 Feb 2, 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.

None yet

3 participants