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.

Sign up for GitHub

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

Jump to bottom

Add new option to fallback custom levels and colors to default values #317

Merged
merged 5 commits into from
Mar 24, 2022
Merged

Add new option to fallback custom levels and colors to default values #317

merged 5 commits into from
Mar 24, 2022

Conversation

bitDaft
Copy link
Contributor

@bitDaft bitDaft commented Mar 19, 2022 •
edited
Loading

Currently the customLevels and customColors options completely override the default Levels.

This PR changes the default behaviour of either using customLevels or default levels, and instead makes it so that it will now override the default levels with new Levels.

The reason for this change is that there is a use case where we may only want to add a few extra custom levels and need to use the existing levels too.

refering to comment in issue #62

mcollina
mcollina requested changes Mar 19, 2022
View reviewed changes
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.

I don't think we can accept this breaking change. Could you add new options instead?

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2009223414

  • 0 of 9 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 1997621969: 0.0%
Covered Lines: 364
Relevant Lines: 364
💛 - Coveralls

@coveralls
Copy link

coveralls commented Mar 19, 2022 •
edited
Loading

Pull Request Test Coverage Report for Build 2011357941

  • 44 of 44 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%
Totals Coverage Status
Change from base Build 1997621969: 0.0%
Covered Lines: 380
Relevant Lines: 380
💛 - Coveralls

@bitDaft
Copy link
Contributor Author

bitDaft commented Mar 19, 2022

Refactored to maintain backward compatibility.

Now uses a new option to switch whether to fallback level and color option to default values

The option name is useOnlyCustomProps, it's default value is true; to get the fallback, false can be passed as value.

The default is kept as true, to keep inline with how pino uses the useOnlyCustomLevels option which needs to be true to override the levels. Maybe this option can be set to default false in the next major version to follow along with pino convention of overriding and adding custom levels

@bitDaft bitDaft requested a review from mcollina March 20, 2022 04:30
mcollina
mcollina requested changes Mar 20, 2022
View reviewed changes
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.

Good work! Could you add the new option to the README and update the PR title?

@bitDaft bitDaft changed the title Fix custom level and colour to append instead of replace Add new option to fallback custom levels and colors to default values Mar 20, 2022
@bitDaft
Copy link
Contributor Author

bitDaft commented Mar 20, 2022

updated

@bitDaft bitDaft requested a review from mcollina March 20, 2022 09:16
mcollina
mcollina approved these changes Mar 20, 2022
View reviewed changes
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

@bitDaft
Copy link
Contributor Author

bitDaft commented Mar 20, 2022

@mcollina can this PR be merged?

@mcollina mcollina merged commit 86715d8 into pinojs:master Mar 24, 2022
@kibertoad kibertoad mentioned this pull request Jul 23, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bitDaft @coveralls @mcollina