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

Shell completions for Cargo #1646

Merged
merged 13 commits into from Apr 14, 2019

Conversation

Projects
None yet
6 participants
@yodaldevoid
Copy link
Contributor

commented Feb 8, 2019

This is a cleanup of a prior PR (#1425) by @ricvelozo.

  • Adds the ability to output shell completions for cargo using the command rustup completions <shell> cargo
  • Updates the completion instructions to mention and suggest placing bash completions in the home directory.
@kinnison

This comment has been minimized.

Copy link
Collaborator

commented Feb 16, 2019

Hi @yodaldevoid

Thanks for your contribution. Could you possibly tidy up the commit sequence (e.g. fold the fixes to formatting into the requisite commits instead of having a 'run rustfmt' commit) to make it easier to review?

I've put this PR on my list to watch but I'd like to see things a little tidier before I review.

I look forward to your changes.

D

@yodaldevoid yodaldevoid force-pushed the yodaldevoid:shell-completions branch from e843bf1 to 49626f9 Feb 16, 2019

@yodaldevoid

This comment has been minimized.

Copy link
Contributor Author

commented Feb 16, 2019

Hello @kinnison

Thanks for getting to looking over this. I've tidied up the rustfmt commit, but no other commits jumped out at me as being appropriate to tidy up. Is there anything else you would like cleaned up?

I should note that I wanted to keep @ricvelozo's commits mostly intact. They could get merged together with other commits, to condense everything, but I wanted to preserve his contribution.

@kinnison

This comment has been minimized.

Copy link
Collaborator

commented Feb 17, 2019

Fair enough, I can appreciate wanting to maintain that. I'll put this on my list to review as soon as I can (likely within a day or two)

@bors

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

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

@yodaldevoid yodaldevoid force-pushed the yodaldevoid:shell-completions branch from 49626f9 to da9db14 Feb 28, 2019

@yodaldevoid

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

@kinnison Just wanted to ping you to make sure this hasn't fallen by the wayside.

@yodaldevoid yodaldevoid force-pushed the yodaldevoid:shell-completions branch from da9db14 to e80e3a5 Feb 28, 2019

@kinnison
Copy link
Collaborator

left a comment

First up, thank you for submitting a PR and contributing to rustup.

This is not a bad PR, though there are a number of points which need cleanup. The big thing is that there's a distinct lack of tests.

Could we please have some basic tests for the new command you're adding?

Thanks,

D.

Show resolved Hide resolved README.md
Show resolved Hide resolved src/rustup-cli/rustup_mode.rs Outdated
Show resolved Hide resolved src/rustup-cli/rustup_mode.rs Outdated
@ehuss

This comment has been minimized.

Copy link

commented Feb 28, 2019

FWIW, I'm proposing a new approach for completions for cargo at rust-lang/cargo#6645. It may not have any impact here, but just wanted to make you aware.

@yodaldevoid

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

@ehuss I'll keep an eye on that discussion, and when/if it gets implemented if no one else does it I'll make sure to submit a PR moving to the new system for newer versions of cargo.

@dwijnand

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

Thanks, @ehuss, I've been meaning to cross link that here.

@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

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

ricvelozo and others added some commits Jun 1, 2018

Move completion command handler to separate function
Signed-off-by: Gabriel Smith <ga29smith@gmail.com>
Add optional "command" argument to rustup completion
This "command" argument allows rustup to output the completion information of
more than just rustup. In this case it adds a cargo completion script.

Signed-off-by: Gabriel Smith <ga29smith@gmail.com>
For cargo completion script, output line to source real script
This allows the cargo script for the current default toolchain to be called,
and if the script is ever updated the user won't have to maually update their
completion script after updating Rust.

Signed-off-by: Gabriel Smith <ga29smith@gmail.com>
Add information to the help output about bash completions
Signed-off-by: Gabriel Smith <ga29smith@gmail.com>

@yodaldevoid yodaldevoid force-pushed the yodaldevoid:shell-completions branch from e80e3a5 to eeb1d4c Mar 16, 2019

Hold all programs with completions in an array
This removes the duplication between the `variants` and `from_str`
implementations.

@yodaldevoid yodaldevoid force-pushed the yodaldevoid:shell-completions branch from eeb1d4c to 02d63fc Mar 20, 2019

@yodaldevoid

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2019

@kinnison Sorry about the long time since the last update. As the famous quote goes, "Life... finds a way to always get in the way of projects."

I believe I have addressed all requested changes. Please let me know if there is anything else you would like modified.

yodaldevoid added some commits Mar 30, 2019

cli: Rework completion error handling
Moves the error reporting from the completion function to the standard
error handling mechanism.

CompletionCommand had to be made fully public, not that this matters for
a binary target. That said, this could have been prevented by
transforming the CompletionCommand into its String form at the error
site rather than at the reporting site. It was chosen to go with this
method as stringly-typed interfaces are generally bad.
test: mock: Add expect_ok_eq function
Compares two different command invocations and panics if their return
code and stdout/stderr don't match.

@yodaldevoid yodaldevoid force-pushed the yodaldevoid:shell-completions branch from eed1d01 to 0312b11 Mar 30, 2019

@kinnison

This comment has been minimized.

Copy link
Collaborator

commented Mar 30, 2019

Thank you for making the updates, I'll endeavour to review this, likely tomorrow.

@yodaldevoid

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

@kinnison I hate to be a hypocrite in this regard, but I wanted to ping you about this PR as it has been some time.

@kinnison

This comment has been minimized.

Copy link
Collaborator

commented Apr 14, 2019

@kinnison I hate to be a hypocrite in this regard, but I wanted to ping you about this PR as it has been some time.

No, that's very fair, I needed a kick up the backside it seems the past two weeks have been busy and somehow this slipped off my TODO without me noticing :(

@kinnison

This comment has been minimized.

Copy link
Collaborator

commented Apr 14, 2019

This looks reasonable to me, I'm grateful for the tests too. I wonder if the user ought to be told about how the completions script they're given may vary with the default toolchain; but ultimately I think that's not worth blocking this for.

@kinnison kinnison merged commit ed08330 into rust-lang:master Apr 14, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@yodaldevoid yodaldevoid deleted the yodaldevoid:shell-completions branch Apr 14, 2019

@sondr3 sondr3 referenced this pull request Apr 23, 2019

Merged

rustup: 1.17.0 -> 1.18.1 #60098

5 of 10 tasks complete

fnichol added a commit to fnichol/bashrc that referenced this pull request Apr 30, 2019

Add Cargo completions via Rustup
References: rust-lang/rustup.rs#1646

Signed-off-by: Fletcher Nichol <fnichol@nichol.ca>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.