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 support for completing cargo update <TAB> #14552

Open
wants to merge 1 commit 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 #14520

Add custom completer for cargo update <TAB>

@rustbot
Copy link
Collaborator

rustbot commented Sep 17, 2024

r? @ehuss

rustbot has assigned @ehuss.
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. Command-update 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 ehuss Sep 17, 2024
get_dependencies_from_metadata()
.unwrap_or_default()
.into_iter()
.map(|dep| clap_complete::CompletionCandidate::new(dep.name_in_toml().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 descriptions

get_dependencies_from_metadata()
.unwrap_or_default()
.into_iter()
.map(|dep| clap_complete::CompletionCandidate::new(dep.name_in_toml().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.

I think this is supposed to be package name, not dependency name (name_in_toml)

Comment on lines +1079 to +1092
fn get_dependencies_from_metadata() -> CargoResult<Vec<Dependency>> {
let cwd = std::env::current_dir()?;
let gctx = GlobalContext::new(shell::Shell::new(), cwd.clone(), cargo_home_with_cwd(&cwd)?);
let ws = Workspace::new(&find_root_manifest_for_wd(&cwd)?, &gctx)?;

let dependencies = ws
.members()
.flat_map(|pkg| pkg.dependencies().into_iter().cloned())
.collect::<HashSet<_>>()
.into_iter()
.collect::<Vec<_>>();

Ok(dependencies)
}
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 finding all direct dependencies but that is insufficient.

We should resolve dependencies and use those. If the resolve fails, we could consider falling back to these.

Some constraints

  • We should probably be quiet about the resolve
  • We shouldn't change the lockfile

We can either updated our GlobalContext to be quiet (should probably go back and do that to all of these completions anyways) and run the resolve in dry-run mode or we hack things up so you can create an ephemeral workspace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can either updated our GlobalContext to be quiet (should probably go back and do that to all of these completions anyways)

Are you referring to updating the GlobalContext(in the main function) within the custom completer function?

If this is what you mean, I think we will face a problem: the custom completer function cannot carry additional parameters. Capturing &mut gctx with a closure would likely be restricted by lifetime limitations. If there are other methods I haven’t considered, please let me know. Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

let gctx = GlobalContext::new(shell::Shell::new(), cwd.clone(), cargo_home_with_cwd(&cwd)?);

For the gctx we have, we should force it to be quiet. The primary-path in the CLI handles that through calling configure. Unsure if there are more direct ways of configuring it. We might also want to call configure anyways as our context is only partially initialized and I've not audited it for any problems we can run into without fully initializing it.

get_dependencies_from_metadata()
.unwrap_or_default()
.into_iter()
.map(|dep| clap_complete::CompletionCandidate::new(dep.name_in_toml().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.

package names may not always be sufficient.

In clap, run

$ cargo update clap
error: There are multiple `clap` packages in your project, and the specification `clap` is ambiguous.
Please re-run this command with one of the following specifications:
  [email protected]
  [email protected]

PackageIdSpec::query::minimize has a crude algorithm for uniquifing things. We should have a more robust form.

  • Put all packages into a map of name -> Vec<Package>
  • For any entries with duplicates but unique versions, report name@version
  • If there are duplicate versions, report the full PackageIdSpec

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your guidance!

@@ -1067,6 +1068,29 @@ fn get_targets_from_metadata() -> CargoResult<Vec<Target>> {
Ok(targets)
}

pub fn get_dependency_name_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
pub fn get_dependency_name_candidates() -> Vec<clap_complete::CompletionCandidate> {
pub fn get_package_candidates() -> Vec<clap_complete::CompletionCandidate> {

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

fn get_dependencies_from_metadata() -> CargoResult<Vec<Dependency>> {
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_dependencies_from_metadata() -> CargoResult<Vec<Dependency>> {
fn get_packages() -> CargoResult<Vec<Dependency>> {

@bors
Copy link
Collaborator

bors commented Sep 17, 2024

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

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. Command-update 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