feat: add completions for --manifest-path
#15225
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @weihanglo (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
A-cli
| .help_heading(heading::MANIFEST_OPTIONS), | ||
| .help_heading(heading::MANIFEST_OPTIONS) | ||
| .add(clap_complete::engine::ArgValueCompleter::new( | ||
| clap_complete::engine::PathCompleter::any().filter(|path: &Path| { |
There was a problem hiding this comment.
Thanks for the help!
Could you clarify what you want to complete for --manifest-path? From the current implementation and your recording, it seems to me that completion candidates are almost everything. That doesn't seem too helpful.
I would suggest complete workspace members as the first step.
There was a problem hiding this comment.
Right now, we offer no completions.
This completes manifests and potential cargo scripts.
Not sure how "workspace members" would work here because we use this to determine the workspace and cargo scripts can't be workspace members.
I see we complete dot files; I wonder if we should exclude that from is_embedded.
The only feedback I have is the third check is redundant
There was a problem hiding this comment.
This completes manifests and potential cargo scripts.
Unsure if this is only your interpretation from code or you worked with them. I would like to see a clearer PR description for what this aims for, as the --mainfest-path completion hasn't yet been discussed in the original issue, and we don't have tests for shell completions.
There was a problem hiding this comment.
How is completing cargo script via is_embedded helpful? It completes almost every file without a file extension.
There was a problem hiding this comment.
Not sure how "workspace members" would work here because we use this to determine the workspace and cargo scripts can't be workspace members.
If the goal is to complete cargo scripts, agree "workspace members don't work. Though I came from what candidates --manifest-path completion should get, and I must miss the discussion somewhere.
There was a problem hiding this comment.
Could you clarify what you want to complete for
--manifest-path? From the current implementation and your recording, it seems to me that completion candidates are almost everything. That doesn't seem too helpful.
wanted to look for *.rs and Cargo.toml in the current_dir , though yu are right for is_embedded function it check for dotfiles, so should i modify the is_embeded function or put a different check in the complete check
There was a problem hiding this comment.
The only feedback I have is the third check is redundant
removed
There was a problem hiding this comment.
I would like to see a clearer PR description for what this aims for
@weihanglo FYI I feel like I would have probably put just as much details in the PR description as this (less since I wouldn't have included a video). This does exactly what it says and I see little room to add to that: this completes every file that looks like a potential manifest path.
How is completing cargo script via is_embedded helpful? It completes almost every file without a file extension.
Our options
- Complete nothing (current nightly behavior): users must manually type every character of the path.
- Complete all files (current stable behavior): users see every file, more than this PR
- Complete every file name that is accepted by
--manifest-path(this PR): this may match files that don't have a manifest inside them, implicit or explicit - Complete only
Cargo.toml: Cargo script users will have to manually type in every of the path
imo completions should err on the side of being overly broad, rather than less, because otherwise users will have to manually type things out
This is still an improvement over the stable behavior because you don't have
foo/C[TAB], seeCargo.tomlandCargo.lockfoo/Ca[TAB], getfoo/Cargo.toml
There was a problem hiding this comment.
@epage's comment lists the current behavior and what've changed looks better. That is the discussion and information I was looking for. Thanks for summarizing it!!
Personally I am fine with any of these options. Just want to check this is well-discussed not something future us need to compile to figure out.
There was a problem hiding this comment.
I put my text in the PR description
Update cargo 11 commits in 1d1d646c06a84c1aa53967b394b7f1218f85db82..2622e844bc1e2e6123e54e94e4706f7b6195ce3d 2025-02-21 21:38:53 +0000 to 2025-02-28 12:33:57 +0000 - Bump `cc` to 1.2.16 to fix `x86` windows jobs in rust-lang/rust CI (rust-lang/cargo#15245) - refactor(tree): Abstract the concept of a NodeId (rust-lang/cargo#15237) - feat: implement RFC 3553 to add SBOM support (rust-lang/cargo#13709) - refactor(tree): Abstract the concept of an edge (rust-lang/cargo#15233) - chore: bump openssl to v3 (rust-lang/cargo#15232) - fix(package): Register workspace member renames in overlay (rust-lang/cargo#15228) - Implemented `build.build-dir` config option (rust-lang/cargo#15104) - feat: add completions for `--manifest-path` (rust-lang/cargo#15225) - chore: semver-check build-rs against beta channel (rust-lang/cargo#15223) - chore: depend on openssl-sys to correctly pin its version (rust-lang/cargo#15224) - chore: dont check cargo-util semver until 1.86 is released (rust-lang/cargo#15222)
What does this PR try to resolve?
Add auto completion for
cargo build --manifest-path <TAB>Related to #14520
Ways this could be handled:
--manifest-path(this PR): this may match files that don't have a manifest inside them, implicit or explicitCargo.toml: Cargo script users will have to manually type in every of the pathGenerally, completions should err on the side of being overly broad, rather than less, because otherwise users will have to manually type things out
This is still an improvement over the stable behavior because you don't have
foo/C[TAB], seeCargo.tomlandCargo.lockfoo/Ca[TAB], getfoo/Cargo.tomlHow should we test and review this PR?
Screencast.from.23-02-25.04.09.17.PM.IST.webm