Conversation
|
So I think always prompting about this is a mistake, since all users will have to care about this. We should either:
I'd prefer the former UX since then there's only one block of prompts. |
|
I've commented on #2330 - I don't think prompting is ever desirable here. |
kinnison
force-pushed
the
update-or-check-on-install
branch
from
29ef83e to
c6caad9
Compare
Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
kinnison
force-pushed
the
update-or-check-on-install
branch
from
c6caad9 to
b02bc1a
Compare
|
@Manishearth How do you feel about this prototype? |
Manishearth
left a comment
Manishearth
left a comment
There was a problem hiding this comment.
If i'm reading this right this unconditionally does the equivalent of rustup update <default> on install unless otherwise specified?
Seems good to me!
|
Yep that's the intent. Okay, @rbtcollins how about you? Are you okay with this? |
|
I'm sorry, I don't know how to test this. I'm sure y'all know what's right for your project tho. This said, I don't understand why you're trying so hard to avoid a prompt given when last I ran it, rustup.sh already shows a prompt? |
|
@mcclure the initial prototype prompted for every install, even if there wasn't an existing installation to update, which means a lot of people need to go through an unnecessary prompt. The current prototype never prompts, which is also fine IMO, it's hard to see a use case for not updating an existing install when you are installing rustup (as opposed to updating it) |
| .use_delimiter(true), | ||
| ) | ||
| .arg( | ||
| Arg::with_name("no-update-default-toolchain") |
There was a problem hiding this comment.
I feel this is a bit verbose. We have --no-self-update; perhaps just --no-update ?
There was a problem hiding this comment.
I will have a ponder. --no-update feels a little odd but the terseness may be okay. I'll reconsider in the face of the rest of the args, it is a bit long yes.
| use std::process::Stdio; | ||
| use std::sync::Mutex; | ||
|
|
||
| macro_rules! for_host { |
There was a problem hiding this comment.
I don't really feel that a used-once macro that just forms a function once, used in a single test is really pulling its weight - but if you feel it is going to be used again I'm cool with it being added.
There was a problem hiding this comment.
I'm intending, once this is merged, to file an e-easy e-mentor help-wanted to move those macros into mock/clitools.rs
4 participants
This is a semi-prototype, but in theory ready-for-showtime attempt at resolving #2330
Opinions from @Manishearth and @mcclure are invited.