Clap derive #1067
Merged
Clap derive #1067
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I've been using this branch locally and just found this by chance: Debugging with execve("/usr/bin/ls", ["ls", "-l", "-h", "-d", "--color=Auto", …where the capitalization of |
epage
reviewed
Sep 6, 2022
src/cli.rs
Outdated
Comment on lines
676
to
652
| .grouped_values_of("exec") | ||
| .map(CommandSet::new) | ||
| .or_else(|| { | ||
| matches | ||
| .grouped_values_of("exec-batch") | ||
| .map(CommandSet::new_batch) |
There was a problem hiding this comment.
side note: Oh, didn't know you use this feature. No one has really been stepping up to advocate for it and finish it up
There was a problem hiding this comment.
Is there anything I could do to help finish it?
There was a problem hiding this comment.
The linked tracking issue mentions the remaining work.
epage
reviewed
Sep 8, 2022
Because that is what is needed by clap 2
epage
reviewed
Sep 9, 2022
There was a problem hiding this comment.
Overall, I'd say the derive code looks idiomatic at this point. I have not done any comparison with the builder code to ensure consistency or done any manual testing to verify it. I have also not examined any of the changes unrelated to switching to derive (like completions) or refactors of the business logic in support of the derive.
src/cli.rs
Outdated
Comment on lines
583
to
597
| // TODO: windows? | ||
| #[cfg(feature = "completions")] | ||
| fn guess_shell() -> anyhow::Result<Shell> { | ||
| let env_shell = std::env::var_os("SHELL").map(PathBuf::from); | ||
| let shell = env_shell | ||
| .as_ref() | ||
| .and_then(|s| s.file_name()) | ||
| .and_then(|s| s.to_str()) | ||
| .ok_or_else(|| anyhow!("Unable to get shell from environment"))?; | ||
| shell | ||
| .parse::<Shell>() | ||
| .map_err(|_| anyhow!("Unknown shell {}", shell)) | ||
| } |
There was a problem hiding this comment.
side note: Huh, this might be neat to have in clap_complete
Comment on lines
+615
to
+618
| #[derive(Copy, Clone, PartialEq, Eq, Debug, ValueEnum)] | ||
| pub enum ColorWhen { |
There was a problem hiding this comment.
side note: we should implement ValueEnum for clap's version of this enum
Closed
This makes the definition of arguments to fd a little more ergonomic, and makes it easier to insure the types for the arguments are consitent.
Make it more like it used to be.
By the time we check if we should strip the cwd, we've already moved the command out of the options, so store if we got that out earlier.
Because that is what clap requires
To simplify the option parsing.
sharkdp
reviewed
Oct 17, 2022
| @@ -0,0 +1,37 @@ | |||
| PROFILE=release | |||
There was a problem hiding this comment.
I liked that we could previously generate everything with a cargo build. Is there a specific reason for switching to a Makefile?
There was a problem hiding this comment.
See discussion here about generating completions in the build.rs file.
Basically, getting this to work with generating the completions from build.rs was getting complicated. So I needed a way to do something after the build finished. There's probably a cargo plugin that could do this (for example https://github.com/phil-opp/cargo-post), but a makefile seemed like the most straighforward way to do it, and probably wouldn't require installing anything new.
sharkdp
reviewed
Oct 17, 2022
|
Awesome work, @tmccombs. Thank you very much! Do we have any before and after comparisons of the |
|
I did look at a diff of the before and after for -h and --help, and there were no significant changes, but that was before upgrading to clap 4.0 , and making some other changes, so it's probably worth doing again. |
In favor of tracking scoped threads in sharkdp#1141
|
Diffs for help outputs: NOTE: the identation appears to be less. in the new output. so I ignore whitespace in the diffs to avoid every line showing up as different -h: --help: @epage would it be worth having an option in the derive api to exclude the first line of the doc comment from the long_help message? |
|
@tmccombs I would really like to merge this, but the additional inclusion of the short help text in the long help text descriptions is a bit unfortunate. Is there any way we can work around this? |
sharkdp
reviewed
Oct 31, 2022
src/cli.rs
Outdated
Comment on lines
87
to
88
| after_help = "Note: `fd -h` prints a short and concise overview while `fd --help` gives all \ | ||
| details.", |
There was a problem hiding this comment.
Note: clap has a changed description for -h and --help (see your diff output) which makes this after_help message somewhat superfluous.
Yeah. I think we can still pass in the short and long descriptions directly as attributes. It's a little less readable, but is probably ok for now. I'll put together a commit for that. |
Maybe you can still leave the long help text as doc-comment and pass in the short one as an attribute? Or the other way around? |
In particular, we specifically use `long_help` instead of doc comments because using doc comments will always trim the "." off the end of the first paragraph, and will include the short help as the first paragraph of the full help.
|
Thank you very much for your work and patience! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Switch to using clap-derive for parsing command line arguments. For most options, this removes some boilerplate.
Open questions: