-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Polishing --help
output
#4132
Comments
Gonna dump some comparisons/notes here and then do a summary of my thoughts once I've seen it all. Here's a before (3.2.17, left) and after (current git tip, right) of minidump-stackwalk on powershell: Fixes I needed to make (fairly painless with the CHANGELOG's advice, nice!):
|
For anyone else, I want to say that I only consider the changelog to be in draft form at the moment plus we'll write a migration guide like we did for clap 3. |
cargo-vet (this one is a bit messier because we have proper subcommands so lining things up is gonna be wonky...): required diff @@ -215,6 +225,18 @@ dependencies = [
PS C:\Users\ninte\dev\cargo-vet> git diff .\src\cli.rs
diff --git a/src/cli.rs b/src/cli.rs
index f19d89d..2ad2914 100644
--- a/src/cli.rs
+++ b/src/cli.rs
@@ -18,7 +18,6 @@ pub enum FakeCli {
#[clap(version)]
#[clap(bin_name = "cargo vet")]
#[clap(args_conflicts_with_subcommands = true)]
-#[clap(global_setting(clap::AppSettings::DeriveDisplayOrder))]
/// Supply-chain security for Rust
///
/// When run without a subcommand, `cargo vet` will invoke the `check`
@@ -30,7 +29,7 @@ pub struct Cli {
// Top-level flags
/// Path to Cargo.toml
- #[clap(long, name = "PATH", parse(from_os_str))]
+ #[clap(long, name = "PATH", value_parser)]
#[clap(help_heading = "GLOBAL OPTIONS", global = true)]
pub manifest_path: Option<PathBuf>,
@@ -48,7 +47,7 @@ pub struct Cli {
pub no_default_features: bool,
/// Space-separated list of features to activate
- #[clap(long, action, require_value_delimiter = true, value_delimiter = ' ')]
+ #[clap(long, action, value_delimiter = ' ')]
#[clap(help_heading = "GLOBAL OPTIONS", global = true)]
pub features: Vec<String>,
@@ -73,7 +72,7 @@ pub struct Cli {
/// How verbose logging should be (log level)
#[clap(long, action)]
#[clap(default_value_t = LevelFilter::WARN)]
- #[clap(possible_values = ["off", "error", "warn", "info", "debug", "trace"])]
+ #[clap(value_parser(["off", "error", "warn", "info", "debug", "trace"]))]
#[clap(help_heading = "GLOBAL OPTIONS", global = true)]
pub verbose: LevelFilter, Looking at subcommand's --help: |
Tip: @@ -30,7 +29,7 @@ pub struct Cli {
// Top-level flags
/// Path to Cargo.toml
- #[clap(long, name = "PATH", parse(from_os_str))]
+ #[clap(long, name = "PATH", value_parser)]
#[clap(help_heading = "GLOBAL OPTIONS", global = true)]
pub manifest_path: Option<PathBuf>, |
Seeing
|
So the fact that I definitely like that subcommands come first! I had been considering a hack to do that in the post-processed markdown version of this output (the impl if you want to see my current horrors, which are almost certainly going to be broken by 4.0 but I begrudge no one but myself for this fact). I definitely like that DeriveDisplayOrder is the default! I am very particular about my output! In this vein I've also been hurting for a way to create extra headings for subcommand families, a la git (cargo-vet has too many subcommands because it has both plumbing and porcelain commands, and it's hard to indicate that some commands are "core functionality, use these every day" and these others are "utilities that are good to know about but you'll probably never need"): I'm a bit sad to lose the splash of color, I find that it helps me quickly scan the verbose help output and breaks up the monotony. That said my opinion isn't strong enough for me to kick up a fuss. ARGS vs Arguments I could take or leave, don't really care either way. (And I've never really understood the motivation of distinguishing arguments from options...) Looking at these screenshots I am reminded that a higher-impact thing clap could be doing with my help messages is doing some cleanup of artifacts in the text that exist for rustdoc's sake (since this is pulling from docstrings which need to be valid markdown). Obviously "add a fuckin' markdown parser to clap" is a big ask, but it's unfortunate (and iirc I had to do some very careful work to not have the bullets in --filter-graph get completely messed up). |
I'm increasingly of the opinion that usage strings basically suck at the best of times, and actually am already hardcoding them away in rust-minidump (3 years ago or whatever clap was deriving absolute nonsense so I just burnt it to the ground). Like I don't know how anyone ever looks at something like this and goes "yes this helps me" lol |
Oh wait oh god I in fact did not do the migration correct, my cli tests now all die with
Why is --verbose of all things the flag that keeps breaking between clap versions x3 |
I might need to play with that to see whats going on. We switched up how we render the name to fix #992 and I'm unsure if your code is needing a change or if this is a bug.
#1553 which I'm hoping to fit into v4 but it could probably be made in any 4.x release once I get the derive macros done for #1807
I agree that color helps. The concerns with color
We do plan to allow people to customize colors during the 4.x release, see #3234
Hey, at least we stopped distinguishing between flags and options! I'll have to give this some thought.
Its a big ask but we need to at least make it possible, even if we make it either opt-in or opt-out: #2389 (which builds on top of the other issue I linked to for
Yes, this is why I still think we should collapse to |
/// How verbose logging should be (log level)
#[clap(long, action)]
#[clap(default_value_t = LevelFilter::WARN)]
#[clap(possible_values = ["off", "error", "warn", "info", "debug", "trace"])]
#[clap(help_heading = "GLOBAL OPTIONS", global = true)]
pub verbose: LevelFilter, See #3855 EDIT: I guess that points back to #3822 (comment) which is a reply to you... |
Yeah I think I was working on migrating to the new 4.0 API and then you added a way to silence the deprecations and I decided to forget about it for now, hoping a better answer would come along. I just did a hacked up TypedParser and some quick fixups to the markdown hack to get it working again. You can see the full snapshot diffs here, if you're curious: mozilla/cargo-vet#316 |
Oh also: big fan of killing the version info at the start of help, it's not very helpful and also makes snapshots tests randomly churn whenever I cut a releases, which is mildly annoying (my code is also riddled with |
Which snapshot solution do you use? With
Why not just drop |
We use cargo-insta in the most naive possible way, because I do want to get notified about any and all changes to output (and insta makes it really easy to go "yep fine"). Version bumps are just the more annoying one, probably because I haven't setup cargo-release for my projects. X3 |
One argument for keeping ALL CAPS is that it mirrors the heading in man pages. Also, I find the underline to hurt the readability of lowercased headings (see how |
In switching to title case for help headings (clap-rs#4123), it caused me to look at "subcommand" in a fresh light. I can't quite put my finger on it but "Subcommand" looks a bit sloppy. I also have recently been surveying other CLIs and they just use "command" as well. All of them are commands anyways, just some are children of others (subcommands) while others are not (root or top-level commands, or just command). Context is good enough for clarifying subcommands from root commands. This is part of clap-rs#4132
I thought I'd record my usage experiments Current:
Same line
git-style
Shell prompt
Overall, I'm leaning towards leaving the usage the same |
This ensures we don't end up with accidental leading or trailing newlines due to help template variables not being used when a section is empty. This is prep for removing name/version from the default template and is part of clap-rs#4132
The |
I saw this wall of text linked on Stackoverflow. I cannot get colors in my help pages to work, even though I have the following: #[clap(author, version, about, arg_required_else_help = true, color=ColorChoice::Always)] How can I get colors back, as it helps to understand the information in the help page way better. |
The very first item in the issue is about that. Other styling was selected instead for clap v4's default (#4117). The item ends with
If you follow that issue, it is still open though it is my priority to work on when I am able to focus more on clap. I am trying to get some stuff done on another project atm. |
The only way to get the colors back is stay on the previous clap version. This is what I do in my new and existing projects, just use clap v3. |
Thank you for the answer. |
I'm having a hard time following this -- I was surprised when I upgraded to 4.x and the color went away. Is the official answer to stick with v3? That seems quite off. Is an option for re-enabling the color for --help in the pipeline? Additionally, was https://no-color.org/ considered? |
From the Issue's description
towards the end is
This is my top clap priority (though I have another project that is a higher priority atm). If this is a blocker for someone then, yes using clap v3 is the answer.
Unsure how this is relevant. #2963 captures the motivation which in summary which led us to be more neutral in our default terminal styling. no-color would not help with that but instead helps with enabling/disabling application styling. I also hope to respect more env variables like this by default in the future. |
@epage Thanks for the quick response! Understood on priority et al., 'tis life!
From the page:
|
It's wild how opposite my point of view is to all the ones voiced above. I agree with many of the comments from #2906 -- I always hated the v3 I've been wondering how much of the difference is people's legitimate personal preference vs people's terminals being differently configured to make one or the other theme look bad. |
@dtolnay thanks for providing another voice. As there is no way to make accurate internet polls on these types of things, its hard to sift through the bias of who is currently speaking. I look forward to the experimentation and the learning that will happen when theming support is in and am curious to see if someone is able to popularize an alternative that meets the needs discussed in #2906. |
After seeing @dtolnay's screenshots I think the terminal / terminal-theme has an huge effect on this. At the provided screenshots the v3 theme looks more ugly and the v4 theme looks much nicer and modern.
Of course this is probably also only a personal preference. I think the |
Except At one point someone requested a clap-specific environment variable to allow users to control this but that is a policy that I don't think clap should take on but application authors. Users interact with the author's application, not clap, and we shouldn't bypass that relationship. This also gets into compatibility issues for the author if they switch parsers. |
Just to be clear around formatting, this is ANSI/Escape code formatting only (ie: color, underlines, bold, so on). I don't want to be a dead horse, but I'd think the following would be the proper path, reflecting what other tools do and
An aside: By using the base 16 ANSI colors, users can control what "red" for example looks like in their terminal emulator; Or in other words, allows user to control what their terminal looks like without you going much at all. |
@NuSkooler while I plan to eventually have
When all a user or developer wanted is to disable color in the help. Yes, we have The path forward is
|
@epage I think that all makes sense. I guess the main thing I'm confused about is why now default to no color -- even when I as a developer told Clap to explicitly utilize it? Most if not all of the modern CLI apps that I can think of that support color at all, default to using that color. |
Because "color" is a common stand-in word for "terminal styling"
I feel like there is something missing in this statement as it comes across as circular to me. |
@epage Firstly, I appreciate your willingness to debate here! I hope I'm not coming off as complaining, it's certainly not my intent. Your work is extremely appreciated. I maintain a number of OSS projects myself and know how it can be! Understood on all notes. FWIW, I'm happy to try and help! I don't know the Clap code base well other than my external usage, but I do know terminals, ANSI, and some of the various standards, caveats, so on around them. I may not be following the last statement about "circular": And to further point out: Instead of disabling colors all together, one can use the "base" ANSI 0-16 colors, allowing the terminal emulator itself (generally exposed to the user in configuration -- you can see this in MS terminal now even!). This allows the user to define what each of the 0-16 resolve to. In other words, if you say set the "Usage" to color 4, my terminal and your terminal may look quite different based on our individual preferences. If you instead set the color to a specific value (using 256 or true color syntax), the terminal will just show that. |
Thats actually more what we need than clap help. My requirements for theming and custom user styled text is that we don't take on a whole terminal styling API in clap. The path forward
I feel like we are talking in circles on this topic which makes me think one of us is missing something about the other's responses. I've previously replied on why I think
Yes, I'm aware. That is one of the reasons I prefer the basic 16 / other effects over 256 and truecolor, we are more likely to be able to adapt to the users theme. However, different themes still optimize for different colors having better contrast or not. People are not going to theme their terminal specifically for clap. If we do use any colors, they likely need to have the widest theme compatibility for contrast, light and dark (I know, I'm one of those weird people who prefers light themes). |
Please post feedback on #3234 |
Thanks for working on bringing colors back @epage! Questions of taste are always tricky.
I find the compact usage useful to quickly remember exact option names, when I have a rough idea but can't remember the exact syntax. Like in this case with But I agree that when the dense options become too many, they become useless, like for snakemake (this goes one for another screen...). |
To prevent usages from being overwhelming is why clap only shows positionals and commands and not options. The downside to overriding the usage is we show that in errors rather than the "smart" usage that adapts to the users arguments. #4191 explores some ideas for improving usage with hints like
There is another issue for flattening the help for subcommands much like |
We are looking to polish the help as part of the v4 release since some parts of an applications help might be dependent on how clap renders things (e.g. to match clap's ALL CAPS help headings, a user might put theirs in ALL CAPS).
So far we have made the following changes:
dimmed: PR 4117 though some more work might be needed to enable colored help, depending on the needsCommand::disable_colored_help(true)
Command::help_template
,Command:subcommand_help_heading
, andArg::help_heading
Command::help_template
[]
for optional PR 4144git
)Command::subcommand_help_heading
andCommand::subcommand_value_name
-h
and--help
: PR 4159<name> <verson>
from the start of--help
: PR 4160--version
exists and when a subcommand doesn't have a version, it feels out of place (seecargo check -h
)Command::about
is unspecified as it leaves a blank line at the top of the help. fix(help): Always trim output #4156 resolves thatCommand::help_template
--help
output #4132 (comment) for alternatives consideredpodman -h
Rejected
<about>
and collapse it toUsage: <usage>
if its a single line (e.g.find --help
)Arg::help_heading
Future improvements
template
#1433help_heading
s #1553Note: While I mentioning non-clap CLIs, I want to be clear that we don't just want to conform to what is out there. We should be aiming to make the defaults as polished as possible which can include taking inspiration from others.
The text was updated successfully, but these errors were encountered: