sharkdp / hexyl

A command-line hex viewer
Apache License 2.0
8.92k stars 227 forks source link

Use `owo-colors` instead of `anstyle` #183

Closed sharifhsn closed 1 year ago

sharifhsn commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.