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

"-c never" and NO_COLOR are not respected for -h and other usage messages #600

Closed
arp242 opened this issue May 23, 2020 · 11 comments · Fixed by #668
Closed

"-c never" and NO_COLOR are not respected for -h and other usage messages #600

arp242 opened this issue May 23, 2020 · 11 comments · Fixed by #668

Comments

@arp242
Copy link

arp242 commented May 23, 2020

When adding -c never I would expect it to also apply to the -h output, but that's still coloured:

screenshot_2020-05-23-18-07-09_border

This also applies to something like fd -c none -v, which is an error but has coloured (hard to read) output:

screenshot_2020-05-23-18-12-03_border

In general, I'd really recommend to be careful with coloured terminal stuff, because what's helpful for one personal might be next-to-unreadable for the next (especially if it's non-configurable).

This is fd 8.1.0 on Void Linux.

@arp242 arp242 added the bug label May 23, 2020
@arp242 arp242 changed the title -c never is not respected for -h "-c never" and NO_COLOR are not respected for -h and other usage messages May 23, 2020
@sharkdp
Copy link
Owner

sharkdp commented May 24, 2020

Thank you for reporting this. That should be fixed, if possible (I'm not entirely sure if clap allows us to read the value of --color=… when using -h, but I would hope so). Things could certainly be changed for NO_COLOR.

@tpiekarski
Copy link

@sharkdp If you did not yet take a look, I'd like to try to debug and offer a PR

@sharkdp
Copy link
Owner

sharkdp commented May 24, 2020

@sharkdp If you did not yet take a look, I'd like to try to debug and offer a PR

sounds great!

@sharkdp
Copy link
Owner

sharkdp commented May 24, 2020

@tpiekarski It's probably not completely trivial. We might need to set AppSettings::DisableHelpFlags and then implement -h/--help ourselves via App::write_help after checking the --color value and setting ColorAlways/ColorNever appropriately.

But maybe I'm missing something.

@tpiekarski
Copy link

@sharkdp Thanks for taking a closer look.
However I'll take my chances today/tomorrow and try to solve this issue.
Should I have questions I know I can ask them here.

@tpiekarski
Copy link

tpiekarski commented Jun 2, 2020

@tpiekarski It's probably not completely trivial. We might need to set AppSettings::DisableHelpFlags and then implement -h/--help ourselves via App::write_help after checking the --color value and setting ColorAlways/ColorNever appropriately.

Indeed it does not look straight forward at all. But I got an idea how it could be accomplished without providing a custom function for -h / --help.

In app.rs#L7 AppSettings::ColoredHelp could be set at runtime, based upon the value provided by --color|-c and for the error message clap offers AppSettings::ColorNever for never using color at all, see settings.rs#L574.

The only issue I did not yet figured out is, how it could be possible to check the argument color without being able to parse the arguments with get_matches(). Got to dig a little deeper into clap. Could this be an alternative approach?

@sharkdp
Copy link
Owner

sharkdp commented Jun 3, 2020

The only issue I did not yet figured out is, how it could be possible to check the argument color without being able to parse the arguments with get_matches(). Got to dig a little deeper into clap. Could this be an alternative approach?

That's why I suggested the approach above. We will not be able to set ColorNever after we already built the App and executed app_matches(). The help text will be printed inside app_matches(). It's already too late then.

It might be worth asking this question in the clap repo (or checking if it has already been addressed), since it seems to be a common use case.

@tpiekarski
Copy link

Alright, I suspected that you checked such solution already. None the less I wanted to take a look into the issue. I am not so much in favor to rewrite App::write_help, but if you want to, please go ahead.

Indeed there is an open issue at clap for the very feature we've been talking about Partial / Pre Parsing a CLI.

What do you think we track this issue and return to this here when it is possible to pre-parsing arguments? To be frank I'm not confident to look into the issue at clap myself - I am not experienced enough with Rust. Maybe you've got the time to take a closer look there.

@sharkdp
Copy link
Owner

sharkdp commented Jun 6, 2020

I guess we could at least implement the NO_COLOR variant of this. If NO_COLOR is set, we can simply pass ColorNever.

@sharkdp
Copy link
Owner

sharkdp commented Jun 8, 2020

@sharkdp
Copy link
Owner

sharkdp commented Dec 6, 2020

fix released in v8.2.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants