-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(help): display manpage for nested commands #16432
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
base: master
Are you sure you want to change the base?
Conversation
525addb to
286129a
Compare
286129a to
8d3fb08
Compare
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 will need an team FCP
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 about completions?
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.
Can clap complete cargo help report fu<tab>? I guess it is not really supported and we might need to auto-complete in dash-joined form.
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.
num_args(1..) are assumed to not be hierarchical so it can't. We don't have external subcommand completers yet, so switching to that wouldn't help.
Hmm, just realized we might not be offering completions for built-in help with the new completion engine. The old one would instantiate help in special way that automatically generated subcommands for every subcommand, recursively.
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.
with the new completion engine
What is this?
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.
The original clap completion system auto-generated completion files files similar to what we hand write in cargo. The new completion system is what Cargo is migrating to and what you wrote code for.
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.
Ah thanks. I thought clap_completion going to get a new engine 😆.
| #[derive(Clone)] | ||
| enum CommandKind { | ||
| Primary, | ||
| /// Contains the primary manpage name | ||
| Alias(String), | ||
| } |
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.
Why do we care to track this?
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.
For getting the original man page dash-joined name.
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.
Made the intent more outstanding by renaming it to ManPageLookup
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.
Still isn't making sense since all we do is immediately
let subcommand = match lookup {
ManPageLookup::Direct => subcommand,
ManPageLookup::RedirectTo(primary) => primary,
};6eb4401 to
459fd78
Compare
This comment has been minimized.
This comment has been minimized.
459fd78 to
ef90c8a
Compare
| ### Manifest Options | ||
|
|
||
| {{#options}} | ||
| {{> options-locked }} | ||
| {{/options}} |
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.
Should we be bothering if these don't do anything
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.
Or we can hide them also in --help text. Either works for me but man page and help text should be aligned.
src/bin/cargo/commands/help.rs
Outdated
| &[OsStr::new(subcommand), OsStr::new("--help")], | ||
| )?; | ||
| } | ||
| if super::builtin_exec(subcommand).is_none() { |
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.
nit: I think in a previous iteration, you had used !option.is_some(). One advantage of that is it makes the fact that this is negative logic upfront.
Thoughts?
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.
Also, is there a reason you swapped the order of the cases instead of doing option.is_some()?
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.
Ah, for an early return later
src/bin/cargo/commands/help.rs
Outdated
| Some(s) => s, | ||
| None => return Ok(false), | ||
| }; | ||
| if super::builtin_exec(subcommand).is_none() { |
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.
nit: another case of option.is_none()
src/bin/cargo/commands/help.rs
Outdated
| if super::builtin_exec(&subcommand).is_none() { | ||
| // If not built-in, try giving `--help` to external command. | ||
| crate::execute_external_subcommand( | ||
| gctx, | ||
| &subcommand, | ||
| &[OsStr::new(&subcommand), OsStr::new("--help")], | ||
| )?; | ||
| } else { | ||
| crate::execute_internal_subcommand(gctx, &[OsStr::new(&subcommand), OsStr::new("--help")])?; | ||
| } |
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.
doesn't this need an early return?
| if !is_valid_builtin_command_path(&args_command) { | ||
| let command_str = args_command.join(" "); | ||
| let err = anyhow::format_err!( | ||
| "no such command: `{command_str}`\n\n\ | ||
| help: view all installed commands with `cargo --list`", | ||
| ); | ||
| return Err(err.into()); | ||
| } |
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.
cargo --list won't help for cargo report foo
ef90c8a to
ab4cf3d
Compare
This comment has been minimized.
This comment has been minimized.
They were missing before
So that when snapshotting, windows target won't be redacted by snapbox
This allows `cargo help` to display manpages for nested subcommand, such as `cargo report future-incompat`. The argument of `cargo help` can be * A single `<COMMAND>`, such as `cargo help build` or `cargo help report-future-incompat` (dash-joined) * Multiple `<COMMAND>`s, such as `cargo help report future-incompat` This dash-joined form helps man-page user learn from existing convention. The multiple commands form is straightforward for normal users.
What does this PR try to resolve?
This allows
cargo helpto display manpages for nested subcommand,such as
cargo report future-incompat.The argument of
cargo helpcan be<COMMAND>, such ascargo help buildor
cargo help report-future-incompat(dash-joined)<COMMAND>s, such ascargo help report future-incompatThis dash-joined form helps man-page user learn from existing convention.
The multiple commands form is straightforward for normal users.
How to test and review this PR?