-
Notifications
You must be signed in to change notification settings - Fork 150
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
feat: gray out meta data object when using --singleLine=true
#161
feat: gray out meta data object when using --singleLine=true
#161
Conversation
Pull Request Test Coverage Report for Build 619166069
💛 - Coveralls |
I am confused, the tests should not have passed with the changes from 6641ce0, I only pushed the tests changes to show that they are failing, before pushing the fix |
Is it possible that on CI the colors are disabled, so testing for colors is impossible? |
I'm not sure if this only affects my code, but it looks like tests for colors in stdout won't work in CI because colors are disabled by default. If the code changes from this PR were removed in future, the tests would still pass in CI (but fail locally) |
You should use https://github.com/pinojs/pino-pretty/blob/master/lib/colors.js and not chalk directly. You'll be able to test with that. |
I'm sorry but I don't get how to use the |
The Line 18 in 657b7ba
You will need to use |
Sorry I've spent another 15minutes on this I just don't know what to do with the Do you want to finish this up yourself? I'll learn from your changes for the next time |
https://runkit.com/embed/op9ez5z3pgxm
What you want to do is add a new method that will colorize according to a different color, e.g. |
I would add that code to |
I would expect to see it defined for each colorizer type in the same general location as the definition of |
see my commit at 2d7ca36 That makes the tests fail locally now. How do I test for the color output? |
pino-pretty/test/lib/colors.test.js Line 42 in 657b7ba
|
okay so I can write a unit test for the |
Is there a need to? If the colorizer is working as tested, then the final result is going to be correct, yes? |
okay, just wanted to make sure. I think it would be better to have some integration tests, because in theory the usage of I think I'm all set now, I'll finish this up. Thank you so much for bearing with me |
--singleLine=true
--singleLine=true
I think this is ready |
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
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
Should a note about the different coloring be added to the documentation for the single line option?
I don't think we should add any note. |
fixes #160
Before
After