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

Terminal coloring wishlist #268

Open
4 tasks
CosmicHorrorDev opened this issue Nov 3, 2023 · 10 comments
Open
4 tasks

Terminal coloring wishlist #268

CosmicHorrorDev opened this issue Nov 3, 2023 · 10 comments
Labels
C-enhancement Category: New feature or request M-help wanted Meta: Extra attention is needed

Comments

@CosmicHorrorDev
Copy link
Collaborator

CosmicHorrorDev commented Nov 3, 2023

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

@CosmicHorrorDev CosmicHorrorDev added C-enhancement Category: New feature or request M-help wanted Meta: Extra attention is needed labels Nov 3, 2023
@nc7s
Copy link
Contributor

nc7s commented Nov 3, 2023

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

@CosmicHorrorDev
Copy link
Collaborator Author

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

@nc7s
Copy link
Contributor

nc7s commented Nov 25, 2023

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
Copy link
Collaborator Author

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
Copy link
Contributor

nc7s commented Nov 26, 2023

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
Copy link
Collaborator Author

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

@nc7s
Copy link
Contributor

nc7s commented Nov 26, 2023

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

@nc7s
Copy link
Contributor

nc7s commented Nov 26, 2023

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

@CosmicHorrorDev
Copy link
Collaborator Author

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
Copy link
Contributor

nc7s commented Nov 26, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: New feature or request M-help wanted Meta: Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants