Terminal coloring wishlist

chmln / sd

Intuitive find & replace CLI (sed alternative)

MIT License

5.72k stars 136 forks source link

Terminal coloring wishlist #268

Open CosmicHorrorDev opened 10 months ago

CosmicHorrorDev commented 10 months ago

Currently our support for terminal coloring is pretty spotty. Ideally we would

[ ] Migrate away from ansi_term since it's been unmaintained for quite a while
[ ] Respect NO_COLOR and FORCE_COLOR (most coloring libraries support this out-of-the-box these days)
[ ] Add a --colo[u]r <when> flag where <when> can be always|auto|never
[ ] Properly handle either stdout or stderr independently being a tty
- A lot of programs naively assume that if stdout is a tty then so is stderr and vice-versa. We should appropriately handle either one being a tty (*cough cough* use something like anstream *cough*)

I was planning on looking into anstream and owo-colors, but anything that can handle all of the above should work just fine

nc7s commented 10 months ago

Oh this is a rather comprehensive listing. I think I can take this with such detail.

CosmicHorrorDev commented 10 months ago

Feel free to take up the work if ya want! I'd be happy to review a PR for it

nc7s commented 9 months ago

There's a kind of chicken and eggs problem: if we want a --color argument, it obviously goes through clap, but clap itself doesn't use that. If it is to be supported for clap, we are gonna dig deep into clap internals.

CosmicHorrorDev commented 9 months ago

we are gonna dig deep into clap internals.

Fortunately doesn't look like we need to dig very deep

https://docs.rs/clap/latest/clap/enum.ColorChoice.html

There just may need to be some extra logic to determine that choice, but passing it to clap is straightforward

nc7s commented 9 months ago

Not completely, there's at least an ClapError::unknown_argument that goes before we can Command::color(). It's constructed and shown during (or right after) the parsing phase, before you can get the value of --color.

CosmicHorrorDev commented 9 months ago

I think it's reasonable enough to ignore --color if there's a CLI parsing failure

nc7s commented 9 months ago

Alright, it's just mildly annoying to be partially inconsistent.

nc7s commented 9 months ago

Besides, IMO --color takes precedence over NO_COLOR & co., because the latter cover more area.

CosmicHorrorDev commented 9 months ago

I agree it would be normal for --color to have higher precedence

I don't want to do any "permissive parsing" of the CLI args though. If there was a failure parsing any of the args then we discard all of that information

nc7s commented 9 months ago

Sure, current usage is just that, clap would handle parsing failure itself and exit before we get the results.