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

feat: gray out meta data object when using --singleLine=true #161

Merged
merged 5 commits into from Mar 5, 2021
Merged

feat: gray out meta data object when using --singleLine=true #161

merged 5 commits into from Mar 5, 2021

Conversation

gr2m
Copy link
Contributor

@gr2m gr2m commented Mar 2, 2021

fixes #160

Before

image

After

image

@coveralls
Copy link

coveralls commented Mar 2, 2021

Pull Request Test Coverage Report for Build 619166069

  • 3 of 3 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 614985914: 0.0%
Covered Lines: 265
Relevant Lines: 265

💛 - Coveralls

@gr2m
Copy link
Contributor Author

gr2m commented Mar 2, 2021

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

@gr2m
Copy link
Contributor Author

gr2m commented Mar 2, 2021

Is it possible that on CI the colors are disabled, so testing for colors is impossible?

@gr2m
Copy link
Contributor Author

gr2m commented Mar 2, 2021

When running this locally I get

image

image

@gr2m
Copy link
Contributor Author

gr2m commented Mar 2, 2021

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)

@mcollina
Copy link
Member

mcollina commented Mar 2, 2021

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.

@gr2m
Copy link
Contributor Author

gr2m commented Mar 3, 2021

I'm sorry but I don't get how to use the colorizer API from colors.js to color the meta object output as gray (debug). Can you give me a pointer plaese

@mcollina
Copy link
Member

mcollina commented Mar 3, 2021

The new chalk.Instance({ level }) API in

const ctx = new chalk.Instance({ level: 3 })
will print out colors even if it's disabled in the TTY.

You will need to use colorizer with the level argument of 'trace' (which is grey). The tricky part is that you'd need to percolate down the current colorizer from the caller function

lib/utils.js Outdated Show resolved Hide resolved
@gr2m
Copy link
Contributor Author

gr2m commented Mar 3, 2021

Sorry I've spent another 15minutes on this I just don't know what to do with the colorizer argument. colorizer has a single method .message() which does not accept a level parameter. And calling colorizer as a function only returns the name of the level, not a chalk-compatible instance, e.g. colorizer('trace') // TRACE

Do you want to finish this up yourself? I'll learn from your changes for the next time

@jsumners
Copy link
Member

jsumners commented Mar 3, 2021

Sorry I've spent another 15minutes on this I just don't know what to do with the colorizer argument. colorizer has a single method .message() which does not accept a level parameter. And calling colorizer as a function only returns the name of the level, not a chalk-compatible instance, e.g. colorizer('trace') // TRACE

Do you want to finish this up yourself? I'll learn from your changes for the next time

https://runkit.com/embed/op9ez5z3pgxm

lib/colors returns a factory function that asks "do you want colors or not?". In either case, it returns a new function that will return a colorized level label based upon the level given, e.g. colorize('trace') or colorize('info'). This function also includes a message method that will colorize a message according to our standardized message color: colorizer.message('a message').

What you want to do is add a new method that will colorize according to a different color, e.g. colorizer.greyMessage = (msg) => colorizeLevel('trace', msg).

@gr2m
Copy link
Contributor Author

gr2m commented Mar 3, 2021

What you want to do is add a new method that will colorize according to a different color, e.g. colorizer.greyMessage = (msg) => colorizeLevel('trace', msg).

I would add that code to lib/colors.js as part of the factory function?

@jsumners
Copy link
Member

jsumners commented Mar 3, 2021

I would add that code to lib/colors.js as part of the factory function?

I would expect to see it defined for each colorizer type in the same general location as the definition of colorizer.message.

@gr2m
Copy link
Contributor Author

gr2m commented Mar 3, 2021

see my commit at 2d7ca36

That makes the tests fail locally now. How do I test for the color output?

@jsumners
Copy link
Member

jsumners commented Mar 3, 2021

That makes the tests fail locally now. How do I test for the color output?

t.is(colorized, '\u001B[90mTRACE\u001B[39m')

@gr2m
Copy link
Contributor Author

gr2m commented Mar 3, 2021

okay so I can write a unit test for the colorizer.greyMessage method, but I cannot test the color of the log output in basic.test.js?

@jsumners
Copy link
Member

jsumners commented Mar 3, 2021

okay so I can write a unit test for the colorizer.greyMessage method, but I cannot test the color of the log output in basic.test.js?

Is there a need to? If the colorizer is working as tested, then the final result is going to be correct, yes?

@gr2m
Copy link
Contributor Author

gr2m commented Mar 3, 2021

okay, just wanted to make sure. I think it would be better to have some integration tests, because in theory the usage of colorizer.greyMessage in prettifyObject() could be removed, but the tests would continue to pass.

I think I'm all set now, I'll finish this up. Thank you so much for bearing with me

@gr2m gr2m changed the title test: gray out meta data when using --singleLine=true feat: gray out meta data object when using --singleLine=true Mar 3, 2021
@gr2m
Copy link
Contributor Author

gr2m commented Mar 3, 2021

I think this is ready

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

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

Should a note about the different coloring be added to the documentation for the single line option?

@mcollina
Copy link
Member

mcollina commented Mar 5, 2021

I don't think we should add any note.

@mcollina mcollina merged commit 20e0c7d into pinojs:master Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gray out meta information when using --singleLine option when coloring is enabled
4 participants