Skip to content
This repository has been archived by the owner on Jun 10, 2022. It is now read-only.

feat: dedupe logs #38

Merged
merged 8 commits into from
Jul 15, 2020
Merged

feat: dedupe logs #38

merged 8 commits into from
Jul 15, 2020

Conversation

robertsLando
Copy link
Contributor

fixes #37

@robertsLando
Copy link
Contributor Author

robertsLando commented Jul 14, 2020

@jsumners @mcollina could you help me understand why tests are failing?

@robertsLando
Copy link
Contributor Author

robertsLando commented Jul 14, 2020

Even if I clone main repo and run tests the same two tests are failing.

creates pretty write stream with custom options for pino-pretty, via prettyPrint (default)

creates pretty write stream with custom options for pino-pretty, via prettyPrint (obsolete)

--- expected                
  +++ actual                  
  @@ -1,2 +1,2 @@             
  -INFO : foo                 
  +[1594740049261] INFO : foo

@robertsLando
Copy link
Contributor Author

The problem is with pino-pretty@4.0.0, if I downgrade to version 3.6.1 tests are working

@robertsLando
Copy link
Contributor Author

I think your tests are working because the cache is not correctly updated for dev dependencies and tests are running using an old prino pretty version. I suggest to fix the ci cache too @mcollina @jsumners. The bug so is not with this PR but it's with pino-pretty version 4+

@mcollina
Copy link
Member

Turns out pinojs/pino-pretty#117 broke the tests here.

cc @jsumners I think pinojs/pino-pretty#117 has a bug, I'll open a PR there.


@robertsLando downgrading to 3.6.1 is definitely not the solution.

@robertsLando
Copy link
Contributor Author

downgrading to 3.6.1 is definitely not the solution.

Was to show you where the bug is, I will wait for the next pino-pretty release 👍

@mcollina
Copy link
Member

pino-pretty v4.0.2 was just released with the fix.

@robertsLando
Copy link
Contributor Author

@mcollina Ready for review

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 requested a review from jsumners July 15, 2020 11:03
Readme.md Show resolved Hide resolved
@robertsLando robertsLando requested a review from jsumners July 15, 2020 11:52
Readme.md Outdated Show resolved Hide resolved
Co-authored-by: James Sumners <james@sumners.email>
Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

LGTM

@robertsLando
Copy link
Contributor Author

Let me know when a new release with this feat will be available, thanks :)

@mcollina mcollina merged commit 0a07088 into pinojs:master Jul 15, 2020
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.

dedupe logs
3 participants