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

Remove log version #623

Merged
merged 1 commit into from
May 25, 2019
Merged

Remove log version #623

merged 1 commit into from
May 25, 2019

Conversation

thomasthiebaud
Copy link
Contributor

@thomasthiebaud thomasthiebaud commented Mar 30, 2019

Fixes #620

Removing v1 breaks a test in pretty.test.js. To fix it I have to update pino-pretty. I wanted to do so, but it seems there is a circular dependency when running tests pino -> pino-pretty -> pino.
How do you usually handle update both packages at the same time?

How should pino-pretty handle it? Removing this check

function isPinoLog (log) {
  return log && (log.hasOwnProperty('v') && log.v === 1)
}

seems to be the easiest way, but then pino-pretty will print logs not coming from pino like that

USERLVL:           
    hello: "world"

@@ -209,7 +209,7 @@ function prettifierMetaWrapper (pretty, dest) {
if (lastLogger.hasOwnProperty(parsedChindingsSym)) {
chindings = lastLogger[parsedChindingsSym]
} else {
chindings = JSON.parse('{"v":1' + lastLogger[chindingsSym] + '}')
chindings = JSON.parse('{' + lastLogger[chindingsSym].substr(1) + '}')
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 not sure about that line

@jsumners
Copy link
Member

I think I’ll just have to find time to cut a new major of pino-pretty sooner than I planned.

@jsumners
Copy link
Member

Work started in pinojs/pino-pretty#58

@jsumners
Copy link
Member

@thomasthiebaud try your branch with pino-pretty@next.

@thomasthiebaud
Copy link
Contributor Author

@jsumners Works like a charm

@jsumners
Copy link
Member

jsumners commented Apr 1, 2019

Cool. It’ll probably be next weekend before I can get back to it.

@davidmarkclements
Copy link
Member

to finish this off we'll need to

  1. get the coverage back to 100
  2. resolve conflicts

@thomasthiebaud
Copy link
Contributor Author

Conflicts resolved. Coverage seems to be back to normal too

@davidmarkclements
Copy link
Member

ok this is part of major, it looks like it might be time to start a v6 branch

@thomasthiebaud
Copy link
Contributor Author

Let me know when you have one, I'll change the PR accordingly

@jsumners
Copy link
Member

pino-pretty@3 is released. You should be able to update the dependency in this PR @thomasthiebaud

@thomasthiebaud
Copy link
Contributor Author

@jsumners Done

@kibertoad
Copy link
Contributor

What is remaining before this PR can be merged?

@mcollina
Copy link
Member

We'll land this when we decide to cut v6 later this year.

@jsumners jsumners changed the base branch from master to next May 25, 2019 13:52
@jsumners jsumners merged commit 2057672 into pinojs:next May 25, 2019
@jsumners
Copy link
Member

next branch has been created and this merged into it.

@thomasthiebaud thomasthiebaud deleted the drop-v-support branch May 25, 2019 16:11
@jonatasbaldin
Copy link

hey there!

do you know when this change will be released?

thanks!

@mcollina
Copy link
Member

mcollina commented Aug 12, 2019 via email

mcollina pushed a commit that referenced this pull request Feb 17, 2020
AleksandrSl pushed a commit to AleksandrSl/pino that referenced this pull request May 11, 2020
Version logger field was removed in pinojs#623 but some places were missed.
mcollina pushed a commit that referenced this pull request May 11, 2020
Version logger field was removed in #623 but some places were missed.
@github-actions
Copy link

github-actions bot commented Feb 4, 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 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants