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

Warn if complete profile is used #2138

Merged
merged 1 commit into from
Nov 26, 2019
Merged

Warn if complete profile is used #2138

merged 1 commit into from
Nov 26, 2019

Conversation

Stupremee
Copy link
Member

This change was requested in #2136.
Whenever a user downloads a toolchain with the complete profile,
a warn message will be printed.

@kinnison kinnison self-assigned this Nov 23, 2019
Copy link
Contributor

@payload payload left a comment

Choose a reason for hiding this comment

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

There is also a rustup set profile complete. The warning would be printed on the next update but not on the set itself. Maybe also implement that set profile warns?

What do you think about small tests for this? I used to look into cli-v2.rs for that.

Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

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

This is indeed the kind of change I was hoping for, so ❤️ for that.

There's a typo in two places which suggests a shared static string would be best.

Also, as @payload suggested, it'd be useful to see a test for this. So to verify the change to the toolchain installation step, we should check that warning is present in at least one test in cli-v2.rs.

One option is to add it to test_complete_profile_skips_missing_when_forced() though perhaps a small test just for this would be best.

In addition, at least one test in cli-inst-interactive.rs should check for the warning in the installation mode. One option would be in install_forces_and_skips_rls() and another would be to create a new test just for this.

I look forward to seeing your updated PR, and thank you again.

src/cli/rustup_mode.rs Outdated Show resolved Hide resolved
src/cli/setup_mode.rs Outdated Show resolved Hide resolved
@Stupremee Stupremee requested a review from kinnison November 24, 2019 12:40
Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

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

The test in cli-v2.rs looks like it ought to be okay, though you don't need the full error message, you could match against warning: downloading with complete profile -- that'd be enough.

tests/cli-inst-interactive.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

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

This looks excellent. Could you please rebase and squash together the commits a little? I'm happy with either one commit for everything, or one for the changes, and one for the tests.

If you can do that, I think we're very close to merging this.

- A warning message will be printed if a toolchain is installed in the
rustup and rustup-init binary
- Add unit tests to test if the warning message appears in rustup mode
and setup mode

Add warning message if toolchain is installed with the complete profile

Add unit tests for warning message

- add unit tests to test if the warning message was printed
- move the warning message string to src/cli/common.rs to centralize the
string

Change executable to rustup-init in test_warn_if_complete_profile_is_used

Fix tests by changing the arguments to rustup-init -y --profile complete in test_warn_if_complete_profile

Shorten check for the error message in test_warn_if_complete_profile_is_used
@kinnison kinnison merged commit 2200a24 into rust-lang:master Nov 26, 2019
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