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

WIP: Add cargo completions command for shell completions. #9288

Closed
wants to merge 1 commit into from

Conversation

PoignardAzur
Copy link

@PoignardAzur PoignardAzur commented Mar 19, 2021

Solves #6645

Should help with #9086. Might help with #8871, #7864, and #5759 if dynamic completion support is added to clap.

The "WIP" part is because this is my first cargo PR and I don't know the conventions. In particular, I don't know how errors should be handled, so the PR has a big nasty .expect right in the middle.

Also, I'm using the imperative syntax because it's what existing code does, but I feel like I should be using #[derive(Clap)] instead? I dunno.

TODO

  • Fix error handling.
  • Fix rustfmt errors.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 19, 2021
@ehuss
Copy link
Contributor

ehuss commented Mar 20, 2021

Thanks for your interest in moving this forward! This isn't quite the direction that I was thinking for #6645. There are some details written in that issue, but in general the idea is that the completion file would delegate all the work to cargo itself. This would likely be a large amount of work, but I think it would be worth it for improved completion support.

@PoignardAzur
Copy link
Author

Right. Either way, I think the only approach that makes sense is to auto-generate the completions from clap, because otherwise you add maintenance to every command to make sure that completions stay up to date with new options and stuff.

There's some use-cases that can't be covered with a static list of completions (eg cargo run --example=); for these use-cases, delegating to cargo makes sense. A good first step might be to add cli options to this PR, eg --list-examples, --list-tests, etc.

On the long term I think using clap's dynamic completions once they're implemented is the most efficient solution.

@bors
Copy link
Contributor

bors commented Mar 22, 2021

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

@ehuss
Copy link
Contributor

ehuss commented Mar 22, 2021

Sorry, I'm not sure I understand why it would "add maintenance to every command". Can you explain that more? I would not expect there to be any required effort for new commands, unless they want to add more intelligent completion beyond the information clap has.

@mainrs
Copy link

mainrs commented Mar 24, 2021

Right. Either way, I think the only approach that makes sense is to auto-generate the completions from clap, because otherwise you add maintenance to every command to make sure that completions stay up to date with new options and stuff.

Isn't that already happening? They call the gen_completions_to , which does exactly that.

On a side note, is there a reason why the name is complete and not something more intuitive like completions? Both names aren't taken on crates, but I feel like complete sounds weird in this context.

@PoignardAzur PoignardAzur changed the title WIP: Add cargo complete command for shell completions. WIP: Add cargo completions command for shell completions. Mar 25, 2021
@PoignardAzur
Copy link
Author

PoignardAzur commented Mar 25, 2021

@sirwindfield

On a side note, is there a reason why the name is complete and not something more intuitive like completions? Both names aren't taken on crates, but I feel like complete sounds weird in this context.

You're right, that was an oversight on my part.

Isn't that already happening? They call the gen_completions_to , which does exactly that.

Can you clarify what you mean? Calling gen_completions_to is what this PR does.

@ehuss

Sorry, I'm not sure I understand why it would "add maintenance to every command". Can you explain that more? I would not expect there to be any required effort for new commands, unless they want to add more intelligent completion beyond the information clap has.

Okay, so, there are two ways to do completions:

  • Generate a bunch of shell commands for each shell; these commands are of the form "when command starts like this, complete it like that".
  • Generate a single cargo autocomplete shell command; which the shell calls every time it want to complete a cargo command.

Right now, the first approach can be auto-generated by clap, which this PR does. You call gen_completions_to and it gives you a bunch of bash or fish or zsh commands or whatever, that you can paste into a config file to be loaded at shell startup.

The second approach doesn't have clap support yet. That means, if you want to go for this approach, you have to add your own metadata to each command (or use clap's metadata somehow, maybe). You need a way for your cargo autocomplete to know that if the command starts with cargo run then it needs to look into options specific to the run subcommand, etc; clap doesn't really provide that data, which is why I said it would "add maintenance to every command".

I've asked around and a clap maintainer says that implementing an auto-complete feature is on the roadmap, but requires internal refactorings. In the meantime, this is the most efficient solution available.

@mainrs
Copy link

mainrs commented Mar 25, 2021

Right now, the first approach can be auto-generated by clap, which this PR does. You call gen_completions_to and it gives you a bunch of bash or fish or zsh commands or whatever, that you can paste into a config file to be loaded at shell startup.

That's what I meant. Your PR already did this. I wasn't aware that the person above was talking about the second approach you just described, the auto suggestion system. Honestly, I think that the current solution is good enough. It's better than nothing, basically introduces no code that needs active maintenance and provides solid completions.

@ehuss
Copy link
Contributor

ehuss commented Mar 25, 2021

My intent was to use the metadata from clap itself so there wouldn't need to be anything additional added. There are some private APIs in clap for fetching the information, which I would consider using.

I wouldn't want to proceed with this approach because it is a regression from the current completion support, which has intelligent completions for various options.

@PoignardAzur
Copy link
Author

I wouldn't want to proceed with this approach because it is a regression from the current completion support, which has intelligent completions for various options.

What do you mean?

@ehuss
Copy link
Contributor

ehuss commented Mar 26, 2021

The existing completions are able to complete values for some options (particularly the bash completions, which are more complete). Some examples:

  • + prefix will auto-complete rustup toolchains.
  • Cargo target names (like --bin …) will complete target names.
  • --target will complete rustc targets when using rustup.
  • Subcommands will auto-complete any installed third-party command-name.
  • --target-dir knows to only complete directories.
  • --manifest-path knows to only complete toml files.

Ideally, built-in completions would improve on the current completions to make these completions more reliable and accurate.

@PoignardAzur
Copy link
Author

PoignardAzur commented Mar 26, 2021

What do you mean, the "existing completions"? As far as I'm aware, the only completions are the ones written by the respective shell maintainers (eg bash, fish, etc).

At the very least I'm the one who updated the fish completions and they were very much incomplete.

EDIT - And just to be clear, it's completely possible to use a mix of auto-generated and manual commands for these completion files that do the smart completions you mention.

@ehuss
Copy link
Contributor

ehuss commented Mar 26, 2021

The completions here, which are included in the distribution in the etc and share directories. I think rustup completions has support for setting them up.

@PoignardAzur
Copy link
Author

Okay, I'm going to be blunt here: I feel like you're arguing against my PR because it doesn't match the vision you had, and not considering the PR on its merits.

If pushing this PR means I have to fight with you to defend it, I'm not going to bother. If you think you have a better idea to achieve the same thing, go ahead and implement it.

As for the file you linked:

  • I'm only seeing a bash completion file; if you really think the config files my PR would generate would be a downgrade, we could just keep the bash files as they are and use it for other shells (which is already what I've done for fish).
  • As I said earlier, there's nothing stopping someone from making a hybrid config file with an auto-generated part and a manually implemented part that does some smart completion with special options.

@ehuss
Copy link
Contributor

ehuss commented Apr 2, 2021

I'm going to close this, as I don't think this is the direction we want to go. A hybrid approach is certainly an option, and if there is interest in pursuing that, I think it can be considered. However, before doing that I would ask for the following:

  • Check out Cargo's development process.
  • Try to understand and research the existing solutions.
  • Support for both forwards and backwards compatibility, whereby cargo-driven completions can be implemented in the future. Old versions should fail gracefully, and conversely future versions should remain compatible.
  • Consider what impact it will have with rustup completions.
  • Follow the unstable process, which almost all new features go through.
  • Provide testing and documentation.

@ehuss ehuss closed this Apr 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants