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

fix: unescape \\ for levelKey #538

Merged
merged 2 commits into from
Nov 8, 2024
Merged

Conversation

eliw00d
Copy link
Contributor

@eliw00d eliw00d commented Nov 4, 2024

closes #456

typeof log[key] === 'boolean'
})
]
.map((key) => key.replaceAll(/\\/g, ''))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This strips off the \\ in log\\.level so that you get the correct key to ignore.

const formatted = pretty(chunk.toString())
t.equal(
formatted,
`[${formattedEpoch}] WARN (${pid}): foo\n`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this fix, you would get log.level underneath in the prettified object.

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

@eliw00d
Copy link
Contributor Author

eliw00d commented Nov 5, 2024

@mcollina Pino 9.0.0 dropped support for Node 14. Is that something that can be done here as well? I can switch to replace if not.

@jsumners
Copy link
Member

jsumners commented Nov 5, 2024

Yes, we should drop 14. It'll be a new major, though.

@eliw00d
Copy link
Contributor Author

eliw00d commented Nov 5, 2024

Yes, we should drop 14. It'll be a new major, though.

Should I make that change in this PR? For example:

      matrix:
        node-version: [18.17, 20, 22]
        os: [ubuntu-latest]
        pino-version: [^9.0.0]

@mcollina
Copy link
Member

mcollina commented Nov 6, 2024

@eliw00d send a fresh PR.

@eliw00d
Copy link
Contributor Author

eliw00d commented Nov 6, 2024

@eliw00d send a fresh PR.

Done.

@eliw00d eliw00d requested a review from mcollina November 7, 2024 15:02
@eliw00d
Copy link
Contributor Author

eliw00d commented Nov 7, 2024

@jsumners @mcollina Should be good to go now! 🎉

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 mcollina merged commit 2a85c0d into pinojs:master Nov 8, 2024
5 checks passed
@eliw00d eliw00d deleted the fix/levelKey branch November 8, 2024 16:22
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.

Regression with levelKey handling in version 10
3 participants