Skip to content

Resolve panic in the console writer for short level field values - #665

Merged
rs merged 1 commit into
rs:masterfrom
ndk:master
Apr 24, 2024
Merged

rs merged 1 commit into
rs:masterfrom
ndk:master

Conversation

@ndk

@ndk ndk commented Apr 9, 2024

Copy link
Copy Markdown
Contributor

This PR fixes an issue in the zerolog console writer. Specifying a non-typical value for the level field, resulting in a serialized value of less than 3 bytes, causes a panic. For instance:

log := zerolog.New(zerolog.NewConsoleWriter())
log.Debug().Str("level", "-").Msg("Hello World")

@ndk ndk changed the title Resolve Panic in zerolog for Short level Field Values Resolve panic in zerolog for short level field values Apr 9, 2024
Comment thread console.go Outdated
}

func consoleDefaultFormatLevel(noColor bool) Formatter {
prefix := func(s string) string {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Why isn't this a top level function?

Comment thread console.go Outdated
Comment on lines +420 to +424
if len(ll) == 0 {
l = unknownLevel
} else {
l = strings.ToUpper(prefix(ll))
}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

What about making all this logic a function instead of just prefix?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done :)

@ndk
ndk marked this pull request as draft April 10, 2024 07:38
@ndk
ndk marked this pull request as ready for review April 10, 2024 07:38
@ndk ndk changed the title Resolve panic in zerolog for short level field values Resolve panic in the console writer for short level field values Apr 10, 2024
@ndk
ndk requested a review from rs April 11, 2024 13:32
Comment thread console.go Outdated

func consoleDefaultFormatLevel(noColor bool) Formatter {
const unknownLevel = "???"
stripLevel := func(ll string) string {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Any reason for this function not to be top level?

@ndk ndk Apr 12, 2024 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

By default, I try to reduce the scope of visibility as much as possible (especially when the stuff is too specific and doesn't look like it's to be reused elsewhere), but I don't have any strong preferences. So, if you think it's better to extract it from the function to the package I'll do it promptly. :)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I prefer smaller unit functions as they enhance readability and do not eliminate the possibility of inlining.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure. Done. :)

@ndk
ndk requested a review from rs April 15, 2024 06:55
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.

2 participants