-
Notifications
You must be signed in to change notification settings - Fork 845
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
Add new err-serializer that serializes all error properties #134
Conversation
Here it is https://github.com/pinojs/pino/blob/master/test/basic.test.js#L70-L90. We can probably move that in its own test file. Be careful that we need to handle also this case https://github.com/pinojs/pino/blob/master/pino.js#L242-L244. |
@mcollina thanks for the hint, still haven't got time to port the tests. I'm currently busy at work, but will try to find some time this weekend. Alternatively, if someone has some time to help I would be much grateful. |
Take your time ;). We are at Node Interactive and we won't have much time either! |
@mcollina I'm wondering if |
Just add a err-serializer.test.js. It's a complex enough issue to have its own test file. |
hey @satazor, any updates on this? |
Hey @mcollina, unfortunately no. I'm starting my own company and it drains all my time. This is not forgotten but I understand that you want to move on with this. What do you suggest? |
@davidmarkclements @jsumners would you have some time to work on this? I'm currently blocked for most of this week. |
Released as v3.1.0. |
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. |
This replaces the core serializer with the one I made in https://github.com/IndigoUnited/pino-err-serializer.
Before merging, I need to port the tests to this repo. Any guidance to were these should be added?
There's https://github.com/pinojs/pino/blob/master/test/serializers.test.js but it doesn't actually test the core serializers.