Skip to content
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.

Sign up for GitHub

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

feat: add completions for --manifest-path #15225

Merged
merged 2 commits into from
Feb 24, 2025
Merged

feat: add completions for --manifest-path #15225

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7f94b33
feat: add completions for `--manifest-path`
Gmin2 Feb 23, 2025
c3c2ea2
removed `is_file` check as it was not needed
Gmin2 Feb 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion src/cargo/util/command_prelude.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use crate::util::important_paths::find_root_manifest_for_wd;
use crate::util::interning::InternedString;
use crate::util::is_rustup;
use crate::util::restricted_names;
use crate::util::toml::is_embedded;
use crate::util::{
print_available_benches, print_available_binaries, print_available_examples,
print_available_packages, print_available_tests,
Expand Down Expand Up @@ -325,7 +326,18 @@ pub trait CommandExt: Sized {
self._arg(
opt("manifest-path", "Path to Cargo.toml")
.value_name("PATH")
.help_heading(heading::MANIFEST_OPTIONS),
.help_heading(heading::MANIFEST_OPTIONS)
.add(clap_complete::engine::ArgValueCompleter::new(
clap_complete::engine::PathCompleter::any().filter(|path: &Path| {
Copy link
Member

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.

Copy link
Contributor

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

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

@Gmin2 Gmin2 Feb 24, 2025 •
edited
Loading

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

Copy link
Contributor

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

  1. Complete nothing (current nightly behavior): users must manually type every character of the path.
  2. Complete all files (current stable behavior): users see every file, more than this PR
  3. 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
  4. 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

  1. foo/C[TAB], see Cargo.toml and Cargo.lock
  2. foo/Ca[TAB], get foo/Cargo.toml

Copy link
Member

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I put my text in the PR description

if path.file_name() == Some(OsStr::new("Cargo.toml")) {
return true;
}
if is_embedded(path) {
return true;
}
false
}),
)),
)
}

Expand Down