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

Add ColorHeaderAndFields logger option #118

Merged
merged 7 commits into from Aug 29, 2022
Merged

Conversation

picatz
Copy link
Contributor

@picatz picatz commented Aug 29, 2022

This PR builds on #108, extending the ability to color fields as well as the level header.

When I have log lines with multiple fields (especially with long values), I find it hard to visually pinpoint the part of the log message I'm interested in. I think by dimming the key= part of the field, it'll be faster and easier to visually parse the information. For multi-line values, this will also dim the | prefix.

Usage

log := hclog.New(&hclog.LoggerOptions{
	Color:                hclog.AutoColor,
	ColorHeaderAndFields: true,
})

Note: if ColorHeaderOnly is also set as true, that takes precedence.

Before

Screen Shot 2022-08-29 at 12 48 07 PM

After

Screen Shot 2022-08-29 at 12 45 21 PM

@picatz picatz requested a review from evanphx August 29, 2022 16:52
Copy link
Contributor

@evanphx evanphx left a comment

Choose a reason for hiding this comment

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

Mostly a lot of questions about formatting and logic

intlogger.go Outdated Show resolved Hide resolved
intlogger.go Outdated
// Optionally apply the ANSI "dim" (faint) and "bold"
// SGR values to the key.
if l.fieldColor != ColorOff {
color := color.New(color.Faint, color.Bold)
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems weird to use faint and bold. Why not just bold?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like faint helps break up a log line visually, for me, better than just bold.

Just Bold

Screen Shot 2022-08-29 at 1 31 52 PM

Bold and Faint

Screen Shot 2022-08-29 at 1 32 25 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

The concern is mostly how other folks default terminal fonts will handle faint+bold. We have to be careful picking here because it has a big influence on all the users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a fair point. I think the Windows terminal would likely be the odd one to consider, and it seems like they do support this style: microsoft/terminal#6703 (comment)

Faint/dim is a fairly standard SGR style, implemented in VSCode's terminal emulator (xtermjs) as well (which is what those screenshots are showing).

Bold, I believe, is the more widely supported style. But, I would really rather have faint than bold here to help break up the line. I feel it's important. I don't know if we can actually detect if it's supported?

intlogger.go Outdated Show resolved Hide resolved
intlogger.go Outdated Show resolved Hide resolved
intlogger.go Outdated Show resolved Hide resolved
intlogger.go Outdated Show resolved Hide resolved
@picatz picatz requested a review from evanphx August 29, 2022 19:07
Copy link
Contributor

@evanphx evanphx left a comment

Choose a reason for hiding this comment

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

Looks great! Be sure to squash the history on merging. One minor question about fainting the = is all.

intlogger.go Outdated Show resolved Hide resolved
intlogger.go Outdated Show resolved Hide resolved
intlogger.go Outdated Show resolved Hide resolved
@picatz picatz merged commit 9846b38 into main Aug 29, 2022
@picatz picatz deleted the color-header-and-fields branch August 29, 2022 19:44
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