Skip to content

Add --default flag in rustup toolchain install#4962

Open
FranciscoTGouveia wants to merge 3 commits into
rust-lang:mainfrom
FranciscoTGouveia:default-flag
Open

Add --default flag in rustup toolchain install#4962
FranciscoTGouveia wants to merge 3 commits into
rust-lang:mainfrom
FranciscoTGouveia:default-flag

Conversation

@FranciscoTGouveia

Copy link
Copy Markdown
Contributor

Based on #t-rustup > Reducing the need for setup-rust actions @ 💬.

Feedback on the wording of the doc comment for the new flag is welcome.

@djc djc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this get some tests?

View changes since this review

Comment thread src/cli/rustup_mode.rs Outdated
@FranciscoTGouveia

FranciscoTGouveia commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Should this get some tests?

I would argue that this change is somewhat trivial, but I wouldn't oppose it.
Should I add a new test?

Comment thread src/cli/rustup_mode.rs Outdated
@FranciscoTGouveia FranciscoTGouveia force-pushed the default-flag branch 2 times, most recently from 8ef4627 to 87dc74b Compare July 15, 2026 11:02

@rami3l rami3l left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM modulo the one remaining concern, thanks :)

View changes since this review

@rami3l

rami3l commented Jul 15, 2026

Copy link
Copy Markdown
Member

Should this get some tests?

I would argue that this change is somewhat trivial, but I wouldn't oppose it. Should I add a new test?

@FranciscoTGouveia Looking back I'd second @djc that it would be useful to add a tiny test for it; probably also for --override if that hasn't been done previously.

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.

4 participants