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.

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 custom completer for completing cargo build --packge <TAB> #14553

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

shannmu
Copy link
Contributor

@shannmu shannmu commented Sep 17, 2024

What does this PR try to resolve?

Tracking issue github.com/rust-lang/cargo/issues/14520

Add custom completer for cargo build --package <TAB>

@rustbot
Copy link
Collaborator

rustbot commented Sep 17, 2024

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot 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 Sep 17, 2024
@shannmu
Copy link
Contributor Author

shannmu commented Sep 17, 2024

r? @epage

@rustbot rustbot assigned epage and unassigned weihanglo Sep 17, 2024
@@ -1067,6 +1073,24 @@ fn get_targets_from_metadata() -> CargoResult<Vec<Target>> {
Ok(targets)
}

fn get_package_candidates() -> Vec<clap_complete::CompletionCandidate> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fn get_package_candidates() -> Vec<clap_complete::CompletionCandidate> {
fn get_ws_member_candidates() -> Vec<clap_complete::CompletionCandidate> {

get_packages_from_metadata()
.unwrap_or_default()
.into_iter()
.map(|pkg| clap_complete::CompletionCandidate::new(pkg.name().as_str()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's include the package descriptions

.collect::<Vec<_>>()
}

fn get_packages_from_metadata() -> CargoResult<Vec<Package>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fn get_packages_from_metadata() -> CargoResult<Vec<Package>> {
fn get_ws_member_packages() -> CargoResult<Vec<Package>> {

@bors
Copy link
Contributor

bors commented Sep 17, 2024

☔ The latest upstream changes (presumably #14535) made this pull request unmergeable. Please resolve the merge conflicts.

Comment on lines 93 to 96
.help_heading(heading::PACKAGE_SELECTION)
.add(clap_complete::ArgValueCandidates::new(
get_ws_member_candidates,
)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like cargo uninstall uses this flag for the same reason as the positional argment

Copy link
Contributor

Choose a reason for hiding this comment

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

arg_package_spec_no_all( calls this

  • cargo tree uses to refer to any package in the dependency graph

arg_package_spec calls that but all uses look good

.help_heading(heading::PACKAGE_SELECTION),
.help_heading(heading::PACKAGE_SELECTION)
.add(clap_complete::ArgValueCandidates::new(move || {
if ["build", "tree"].contains(&name.as_str()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is subtle and going to be very confusing. I wonder if we should better organize the arguments by what kind of package is being requested

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While implementing the completion, I discovered that parameters with the same name require different values under different subcommands. To minimize disruption to the original code, I added the _name interface, which allows the subcommand being used to be determined at compile time. Is there a better way to determine what kind of package is being requested?

Copy link
Contributor

Choose a reason for hiding this comment

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

Some ideas I thought of

  • We could have different arg_package functions for different roles
  • We could accept a completer as an argument

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cli Area: Command-line interface, option parsing, etc. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants