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

Highlight doesn't reset style for trace/trace color is unreadable #184

Closed
judemille opened this issue Oct 1, 2020 · 9 comments
Closed

Comments

@judemille
Copy link
Contributor

#167 added a color for trace, but didn't add a match arm to reset the style. This results in all text after a highlighted trace being black. Also of note is that in a dark terminal, the trace highlight is unreadable. I would suggest putting no coloring on it at all if it's going to be black or white.

@estk
Copy link
Owner

estk commented Oct 1, 2020

@IceSentry any idea about this?

@IceSentry
Copy link
Contributor

@judemille do you highlight the entire line when logging?

The PR was meant to use the same color as env_logger because the original weren't distinctive enough (red for ERROR and WARN), which look fine in a dark themed terminal if it's not the entire line that's being highlighted. I think the best fix for this would be to just make it configurable. As a temporary fix you might want to not highlight the entire line, but I think it makes sense even for trace to have a specific highlight color, but the default could easily be different or None.

This is how it looks for me with env_logger in a dark themed terminal. And this is also how it looked for me with my PR.

image

@judemille
Copy link
Contributor Author

My config is "{h({l})} {d} {t} {M} - {m} {n}"
image
image

@IceSentry
Copy link
Contributor

I'm not sure just removing the color is a fix here. I understand my original PR didn't take into account highlighting the entire line or a very dark theme in general, but it would be better to have a way to change the color. I agree the lack of a reset was my mistake though.

@judemille
Copy link
Contributor Author

I'm not sure there's a better way to go about it easily.

@IceSentry
Copy link
Contributor

IceSentry commented Oct 4, 2020

Yeah, I don't think there's an easy way to add configuration. I think your PR is probably good enough for now. I was just really annoyed by ERROR and WARN having different shades of red which is why I made the original PR, but TRACE is fine with default colors

@estk
Copy link
Owner

estk commented Oct 5, 2020 via email

@IceSentry
Copy link
Contributor

@estk I added a comment of something that could be changed, but the PR works for me so I approved it.

@estk
Copy link
Owner

estk commented Mar 30, 2021

This has been addressed in #186

@estk estk closed this as completed Mar 30, 2021
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

No branches or pull requests

3 participants