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: Add context to level customPrettifier #493

Closed
wants to merge 2 commits into from

Conversation

FoxxMD
Copy link
Contributor

@FoxxMD FoxxMD commented Feb 22, 2024

Currently the customPrettifier for level only returns the numeric level value (if levelKey is not used) which means that anyone who wants to modify value returned has to re-implement the levels pino-pretty already has access to as well as calculate the label to use, which pino-pretty also already does.

This PR include both the "final" log label as well as the colorized output, if applicable, to level prettifier function to give users more control over level output.

It has the additional benefit of enabling users to handle their own level alignment (#489 #140) since they have access to the final string values for level. As example implementation for aligning with spaces:

{
  customPrettifiers: {
        level: (out, label, colorized) => {
            // pad to fix alignment
            // assuming longest level is VERBOSE
            // and may be colorized
            const paddedLabel = label.padEnd(8)
            if(colorized !== label) {
                const padDiff = paddedLabel.length - label.length;
                return colorized.padEnd(colorized.length + padDiff);
            }
            return paddedLabel;
        }
    },
}

Include both the "final" log label as well as the colorized output, if applicable, to `level` prettifier function to give users more control over level output.
@FoxxMD
Copy link
Contributor Author

FoxxMD commented Feb 22, 2024

The only thing missing is that the Typescript typings for all customPrettifiers use Record<string, (inputData: string | object) => string> so TS reports errors when the end user uses level: (out, label, colorized) => { ... and im not sure how to address this.

@FoxxMD
Copy link
Contributor Author

FoxxMD commented Feb 22, 2024

Modified typings...it should work now

@FoxxMD
Copy link
Contributor Author

FoxxMD commented Feb 26, 2024

Related to #494 -- providing colors to all customPrettifier funcs would add another argument to this function signature which I'm not super jazzed about.

If I can get feedback on #494 and this cohesively -- or if ya'll would prefer I close both of these and open a new PR with everything combined -- let me know. But I'm thinking:

Change signature for customPrettifier to be (value: string | object, extras: object) so that all these additional variables can be destructured from the second arg to avoid requiring the user to know argument order for multiple signatures.

EDIT: A combined branch example of providing colors to all prettifiers -- see usage under customPrettifiers section in readme https://github.com/FoxxMD/pino-pretty/tree/additionalFunctionality?tab=readme-ov-file#options

FoxxMD added a commit to FoxxMD/pino-pretty that referenced this pull request Feb 26, 2024
Same implementation as pinojs#493 but using object for extras argument for compatibility with future signature expansion
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
Copy link
Member

@jsumners could you take a look?

@FoxxMD
Copy link
Contributor Author

FoxxMD commented Feb 29, 2024

Which part @mcollina ? This PR or the changes I mentioned above in additionalFunctionality branch? #493 (comment)

@mcollina
Copy link
Member

@FoxxMD sorry I don't understand the question.

@FoxxMD
Copy link
Contributor Author

FoxxMD commented Feb 29, 2024

Please...read my comment above and respond to it. #493 (comment)

For these changes and #494, after I opened these PRs, I found the signatures to be unwiedly

providing colors to all customPrettifier funcs would add another argument

I built out another branch, additionalFunctionality that includes functionally the same changes as this and #494 but with a more normalized signature for both to address this.

Take a look at the customPrettier section on the readme in the additionalFunctionality branch or the diff to see what changes I made.

Per my comment above, the question:

Can I get feedback on #494 and this cohesively -- or would ya'll prefer I close both of these and open a new PR with everything combined (from the additionalFuntionality branch)

"lgtm" reads as if you didn't see this.

@mcollina
Copy link
Member

I missed that. Please send one PR thanks.

@FoxxMD FoxxMD closed this Feb 29, 2024
mcollina pushed a commit that referenced this pull request Mar 20, 2024
…rmat (#495)

* feat: Add label and colorized output for level customPrettifier func

Same implementation as #493 but using object for extras argument for compatibility with future signature expansion

* feat: Provide colorette object to message format function

Enables users to use available colors based on `colorize` context of the pino-pretty instance

Variant of #494 that provides colors as extras object for compatibility with future customPrettifier extras argument

* feat: Provide colorette object to prettifyObject

All keys for customPrettifier, other than logs, now provide colors as property on an additional `extras` object for the prettifyObject function signature

* feat: Add colors to level prettifier function signature

* Modify level prettifier function signature to match other prettifiers
  * Since only the first argument was documented anyway this shouldn't be a breaking change.

* docs: Update docs to showcase colors and standard customPrettifiers function signature

* fix: Missing colors on colorizer when custom level colors are provided
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.

None yet

2 participants