feat: add completions for `--manifest-path` by Gmin2 · Pull Request #15225 · rust-lang/cargo (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation11 Commits2 Checks21 Files changed

Conversation

This file contains hidden or 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

[ Show hidden characters]({{ revealButtonHref }})

What does this PR try to resolve?

Add auto completion for cargo build --manifest-path <TAB>

Related to #14520

Ways this could be handled:

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

Generally, 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], see Cargo.toml and Cargo.lock
foo/Ca[TAB], get foo/Cargo.toml

How should we test and review this PR?

Screencast.from.23-02-25.04.09.17.PM.IST.webm

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 (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

rustbot added A-cli

Area: Command-line interface, option parsing, etc.

S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Feb 23, 2025

.help_heading(heading::MANIFEST_OPTIONS),
.help_heading(heading::MANIFEST_OPTIONS)
.add(clap_complete::engine::ArgValueCompleter::new(
clap_complete::engine::PathCompleter::any().filter(\|path: &Path

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is completing cargo script via is_embedded helpful? It completes almost every file without a file extension.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only feedback I have is the third check is redundant

removed

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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], see Cargo.toml and Cargo.lock
foo/Ca[TAB], get foo/Cargo.toml

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.