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

Use owo-colors instead of anstyle #183

Merged
merged 8 commits into from
Dec 7, 2022
Merged

Use owo-colors instead of anstyle #183

merged 8 commits into from
Dec 7, 2022

Conversation

sharifhsn
Copy link
Contributor

@sharifhsn sharifhsn commented Dec 6, 2022

Closes #179.

owo-colors was suggested as a replacement for anstyle by @mainrs because it exposes color support determination. After investigating this crate more, it turns out that it does this using the supports-color crate. The way that owo-colors exposes this ability does not work for my performance improvement, so I opted to use the supports-color crate directly.

However, while I was looking around the owo-colors docs, I found it to be much more ergonomic for exposing raw color codes as const. I've therefore replaced anstyle with owo-colors and confirmed that it has the same output.

I've also made color=auto default because supports-color has made it more powerful at determining color support.

@sharkdp
Copy link
Owner

sharkdp commented Dec 6, 2022

Thank you for looking into this!

so I opted to use the supports-color crate directly.

Ok. Do we need the "full" functionality of this crate (checking for truecolor support etc.)? Does it provide anything additional on top of env::var_os("NO_COLOR").is_some()?

I've also made color=auto default because supports-color has made it more powerful at determining color support.

Well the main use-case that I want to support by making always the default (which, admittedly, looks like an error.. but is really on purpose) is piping into a pager. hexyls main selling point is the colorized output. So it would be a real shame to force users to use --color=always in case they want to pipe to less, for example.

@sharifhsn
Copy link
Contributor Author

Well the main use-case that I want to support by making always the default (which, admittedly, looks like an error.. but is really on purpose) is piping into a pager.

That's a little bit unusual, from my perspective. I didn't actually know that colored output worked when piping; I assumed it didn't because the previous auto check would base coloring on if it was a TTY or not. It makes more sense to me for auto to use terminal support and environment variables than whether it's piped into a pager.

With that in mind, I would construct the auto option as if it either supports colors in general through supports-color, or if it's piped i.e. not a TTY. That might seem a bit backwards but considering that paging colored output is an intended outcome I think it might be best.

@sharkdp
Copy link
Owner

sharkdp commented Dec 7, 2022

I assumed it didn't because the previous auto check would base coloring on if it was a TTY or not. It makes more sense to me for auto to use terminal support and environment variables than whether it's piped into a pager.

That's how --color=auto works for a lot of tools. Try for example grep --color=auto … | cat or ls --color=auto … | cat. Both will not enable colors. This is not just useful for piping into other processes, but also if you want to pipe to a file. You typically don't want ANSI escape codes to end up in there.

hexyl behaves the same way when using --color=auto. It's just that we don't use this by default. For the reason given above.

Now the question is of course what to do with NO_COLOR. This is actually answered on https://no-color.org/, in FAQ item 2. I interpret this as suggesting that --color=… should overwrite NO_COLOR. So if a user has NO_COLOR set, but uses --color=auto or --color=always, hexyl should behave as if NO_COLOR was not present.

src/bin/hexyl.rs Outdated Show resolved Hide resolved
Co-authored-by: David Peter <sharkdp@users.noreply.github.com>
Copy link
Owner

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

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

Thank you very much!

@sharkdp sharkdp merged commit 4833c2a into sharkdp:master Dec 7, 2022
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.

Add support for the NO_COLOR environment variable
2 participants