-
Notifications
You must be signed in to change notification settings - Fork 18
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
Making pino-debug compatible with debug v3 #11
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 2 2
Lines 44 48 +4
=====================================
+ Hits 44 48 +4
Continue to review full report at Codecov.
|
test/index.js
Outdated
@@ -303,3 +303,17 @@ test('results in valid syntax when source has trailing comment', (t) => { | |||
t.doesNotThrow(() => require('./fixtures/trailing-comment')) | |||
t.end() | |||
}) | |||
|
|||
test('Set env.DEBUG and captures any calls to `debug` and passes them through pino logger', (t) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we rephrase the test: something like
"preserves DEBUG
env independently from debug
module"
hey @lukaszewczak - benchmarks are fixed on master now (we're relying on knowing the debug folder structure, if it changes the benchmarks break, very brittle I know but we have to be able to clear debug module files from the require cache between benchmarks) see #12 |
hey @davidmarkclements , I rephrase the test and I merge you changes and change a little code in benchmarks to use special NULL device on Windows platform based on (https://stackoverflow.com/questions/11053977/how-can-i-write-to-the-nul-device-under-windows-from-node-js). Now benchmarks works for me on Windows and on Linux platform run through docker container. On Windows I got those results:
And on Linux using this
I receive those results:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@lukaszewczak thanks so much for your efforts on this, I've added as a collaborator - no pressure to accept – just if you'd like to continue maintaining |
Thank you @davidmarkclements for this invitation, this will be my first time as collaborator, but i love new challenges and I would like to maintain |
Hi @davidmarkclements ,
So this is my first try to make pino-debug compatible with debug v3.
I need to fix situation when someone add namespaces to debug on
process.env.DEBUG
and after runningpino-debug
those namespaces was erased, because of the debug package behavior which erases any previous namespaces after call todebug.enable
method. This why I add one new test to test this situation, and add new methodgetNamespaces
which retrive namespaces fromprocess.env.DEBUG
.Because debug package erases previous namespaces I put logic into
pinoDebug
method to collect all namespaces ( to enable and disable) and run only oncedebug.enable
method.I had problem with running
npm run bench
script for becnhamrk. After running it I receive this response:Without any errors or results, I even make this test on the master branch without mine changes, and the result is the same. So is this a correct situation?
Regards,
Łukasz