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

Print default toolchain on rustup default without arguments #1633

Merged
merged 1 commit into from
Feb 11, 2019

Conversation

das-sein
Copy link
Contributor

@das-sein das-sein commented Feb 3, 2019

Implementation of #1632

Behavior: When rustup default is not provided arguments, print the default toolchain (new behavior) before commencing updates of all installed toolchains.

@dwijnand
Copy link
Member

dwijnand commented Feb 3, 2019

Hey, @lazorgator. Thanks for sending the PR. Want to update the tests so we can assess how this implementation behaves?

@das-sein
Copy link
Contributor Author

das-sein commented Feb 3, 2019

Yep, sorry about that! Working on them now for both PRs.

@das-sein
Copy link
Contributor Author

das-sein commented Feb 3, 2019

@dwijnand Should be all set. I only needed to modify one test. However, I noticed that it was possible to be in a state wherein no default toolchain was set in the settings. This required modification of the config.get_default() behavior very slightly. It doesn't seem to affect how the rest of the software expects that method to work. Without the modification it would throw an exception about trying to unwrap() a None during the rustup_self_update_exact test.

Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

Thanks, @lazorgator. The patch seems reasonable to me. I'd merge it if I could. So let's see if this matches @nrc's expectations in #1632.

@dwijnand
Copy link
Member

dwijnand commented Feb 4, 2019

r? @nrc

@nrc
Copy link
Member

nrc commented Feb 10, 2019

I'm very sorry, but I messed up and wrote update when I meant default in #1632 . Could you change this PR to printing the same message but on the different command please?

@das-sein das-sein force-pushed the 1632-rustup-print-default branch 2 times, most recently from 5ac007f to 8babd1a Compare February 10, 2019 09:25
@das-sein
Copy link
Contributor Author

@nrc Alright, added the functionality to rustup default as well! If you want me to remove the functionality from rustup update, let me know.

@dwijnand
Copy link
Member

Perhaps it's best to remove it from rustup update.

@das-sein das-sein changed the title Print default toolchain on rustup update without arguments Print default toolchain on rustup default without arguments Feb 10, 2019
@das-sein
Copy link
Contributor Author

das-sein commented Feb 10, 2019

@dwijnand @nrc Should be all set, I think. Functionality removed from rustup update.

Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

Re-LGTM

@nrc
Copy link
Member

nrc commented Feb 11, 2019

Awesome, thanks!

@nrc nrc merged commit 194026d into rust-lang:master Feb 11, 2019
@dwijnand
Copy link
Member

Does this duplicate rustup show active-toolchain?

@das-sein
Copy link
Contributor Author

@dwijnand It does I think! I'll work on a PR to just make the rustup default behavior an alias to that command, if possible. rustup show active-toolchain also deals with the override display issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants