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

Multiple CLI cleanups #419

Merged
merged 9 commits into from May 10, 2016

Conversation

Projects
None yet
4 participants
@brson
Copy link
Contributor

brson commented May 8, 2016

This adds help text for most subcommands and cleans them up in various ways.

The most important thing this does is try to resolve the bikesheddy problem of command synonyms that has been bugging me. That is, should the command be 'install' or 'update'? Should all install-like commands be the same verb or not?

From the commit, this is what I'm going for:

This reorganizes the verbs used in rustup commands. The concept here
is that every subcommand uses the most appropriate 'add/remove' verb
pair for itself, but implements hidden alternate commands using
the obvious other verbs.

So now the problematic verbs are:

  • toolchain install / toolchain uninstall
  • target add / target remove
  • override set / override unset

Additional (hidden) synonyms are:

  • toolchain update
  • toolchain add / toolchain remove
  • target add / target remove

This leaves the top level rustup update as the only non-hidden
'update' command, which has the unique focus of updating everything.

@aturon Importantly, this means that in the README I'm showing rustup toolchain install $toolchain for the installation command, and rustup update for doing updates. I like emphasizing the 'toolchain' in early examples of install command because it's an important concept. This is slightly different than the blog post you have in your hands where I'm demonstrating the (shorter but non-existant) rustup install command, which I have not implemented here.

@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented May 8, 2016

I didn't make it clear but the hidden synonyms aren't just compromise, but some are needed because I changed some of the 'preferred' verbs.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented May 8, 2016

I wonder if it'd be worth to also have a dedicated rustup install synonym for rustup toolchain install? That would perhaps provide a nice easy vector for newbies and would also align with other systems like nvm/rvm/etc?

@kbknapp

This comment has been minimized.

Copy link
Contributor

kbknapp commented May 8, 2016

clap-rs/clap#469 may be applicable too. It's something I hadn't prioritized, but would be a very quick addition.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented May 9, 2016

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

brson added some commits May 7, 2016

Add `target install` / `target update`
These are just the fully names of the `rustup install`
and `rustup update` commands.
Update README for `install` command
This emphasizes `rustup toolchain install` for installing
toolchains, reinforcing the 'toolchain' nomenclature;
leaving `rustup update` for updating everything.
Reorder subcommands per my preferences
'show', 'update', 'default', 'toolchain', 'target', etc.
Tweak the command verbs and establish 'synonyms'
This reorganizes the verbs used in rustup commands. The concept here
is that every subcommand uses the most appropriate 'add/remove' verb
pair for itself, but implements hidden alternate commands using
the obvious other verbs.

So now the problematic verbs are:

* `toolchain install` / `toolchain uninstall`
* `target add` / `target remove`
* `override set` / `override unset`

Additional (hidden) synonyms are:

* `toolchain update`
* `toolchain add` / `toolchain remove`
* `target add` / `target remove`

This leaves the top level `rustup update` as the only non-hidden
'update' command, which has the unique focus of updating everything.

@brson brson force-pushed the brson:cli branch from 48966a1 to 4f91f61 May 9, 2016

@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented May 9, 2016

@alexcrichton I added rustup install <toolchain> as a hidden synonym.

@brson brson force-pushed the brson:cli branch from 4f91f61 to 26fc2d7 May 9, 2016

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented May 9, 2016

Sounds good to me! Maybe some of the README could be updated as well? Other than r=me though, this all sounds good to me

@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented May 9, 2016

Well I made rustup install a hidden synonym so it works, but not with the intent of promoting it in for use, though I do agree that rustup install nightly is nice and sweet, and familiar to rbenv users. Have to think about it a bit more.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented May 9, 2016

Ah ok, either way sounds good to me

@brson

This comment has been minimized.

Copy link
Contributor Author

brson commented May 10, 2016

I went ahead and put the sweet commands in the README (but still not in --help).

@brson brson merged commit c8f6b09 into rust-lang:master May 10, 2016

@bors bors referenced this pull request May 10, 2016

Merged

Configurable host triple #421

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.