-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
clap/locale: fix the colors for all programs (Closes: #8501) #8542
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
Conversation
|
Some GNU tests will fail. |
|
GNU testsuite comparison: |
3c8fe44 to
2317d7f
Compare
|
GNU testsuite comparison: |
2317d7f to
7af285f
Compare
|
GNU testsuite comparison: |
5e23a1b to
12220a7
Compare
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
| let colors_enabled = match color_choice { | ||
| clap::ColorChoice::Always => true, | ||
| clap::ColorChoice::Never => false, | ||
| clap::ColorChoice::Auto => { | ||
| use std::io::IsTerminal; | ||
| IsTerminal::is_terminal(&std::io::stdout()) | ||
| && std::env::var("TERM").unwrap_or_default() != "dumb" | ||
| } | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This snippet is almost identical with the should_use_color_stderr function, the only difference is in what is passed to is_terminal. Maybe should_use_color_stderr can be made more flexible so that it can be used here, too?
|
GNU testsuite comparison: |
|
|
||
| let help_text = err.render().to_string(); | ||
| /// Manages color output based on environment settings | ||
| struct ColorManager(bool); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm undecided whether a ColorManager concept is necessary or a bool property in ErrorFormatter would be enough.
| .override_usage(format_usage(&translate!("printenv-usage"))) | ||
| .infer_long_args(true) | ||
| .infer_long_args(true); | ||
| uucore::clap_localization::configure_localized_command(cmd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for splitting the setup of command into two parts? Why not doing the localization at the end?
| .override_usage(format_usage(&translate!("runcon-usage"))) | ||
| .infer_long_args(true) | ||
| .infer_long_args(true); | ||
| uucore::clap_localization::configure_localized_command(cmd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for splitting the setup of command into two parts? Why not doing the localization at the end?
| .override_usage(format_usage(&translate!("truncate-usage"))) | ||
| .infer_long_args(true) | ||
| .infer_long_args(true); | ||
| uucore::clap_localization::configure_localized_command(cmd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for splitting the setup of command into two parts? Why not doing the localization at the end?
| .infer_long_args(true) | ||
| .after_help(translate!("uniq-after-help")) | ||
| .after_help(translate!("uniq-after-help")); | ||
| uucore::clap_localization::configure_localized_command(cmd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for splitting the setup of command into two parts? Why not doing the localization at the end?
| .override_usage(format_usage(&translate!("who-usage"))) | ||
| .infer_long_args(true) | ||
| .infer_long_args(true); | ||
| uucore::clap_localization::configure_localized_command(cmd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for splitting the setup of command into two parts? Why not doing the localization at the end?
cakebaker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done :)
|
GNU testsuite comparison: |
77003fa to
35381dd
Compare
|
GNU testsuite comparison: |
35381dd to
e6a8666
Compare
|
GNU testsuite comparison: |
251e258 to
ee0fae5
Compare
|
GNU testsuite comparison: |
Co-authored-by: Daniel Hofstetter <[email protected]>
ee0fae5 to
23f3551
Compare
|
GNU testsuite comparison: |
|
Good work :) |
and add tests to make sure we don't regress in the future