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

shows (override) in toolchain list #2312

Merged
merged 1 commit into from
May 30, 2020
Merged

shows (override) in toolchain list #2312

merged 1 commit into from
May 30, 2020

Conversation

bestia-dev
Copy link
Contributor

@bestia-dev bestia-dev commented Apr 28, 2020

This PR is for issue #2295
The command list now shows the override toolchain for this specific directory):

$ home/bin/rustup toolchain list
stable-x86_64-unknown-linux-gnu (default)
nightly-x86_64-unknown-linux-gnu

$ home/bin/rustup override set stable

$ home/bin/rustup toolchain list
stable-x86_64-unknown-linux-gnu (default) (override)
nightly-x86_64-unknown-linux-gnu

$ home/bin/rustup override set nightly

$ home/bin/rustup toolchain list
stable-x86_64-unknown-linux-gnu (default)
nightly-x86_64-unknown-linux-gnu (override)

@kinnison
Copy link
Contributor

The examples look good, but I'd like to see some tests added to check all the cases (e.g. override by rust-toolchain vs. override by path etc.)

I think the tests probably belong in cli-misc.rs

@bestia-dev
Copy link
Contributor Author

bestia-dev commented May 5, 2020

I found a similar test in cli-rustup.rs: list_default_toolchain()
So there I added 2 tests for list override:

  • list_override_toolchain()
  • list_default_and_override_toolchain()
    You mentioned "override by path". Those are different kinds of override.
    If I understand correctly, all of them are "overrides" until we don't get deeper into more details.

@bestia-dev
Copy link
Contributor Author

I think OverrideReason is not needed for the rustup toolchain list command.
The user is informed about the override.
For more details he could find elsewhere maybe in rustup override list?
But if needed I could add the OverrideReason to the string "(override)":

#[derive(Debug)]
pub enum OverrideReason {
    Environment,
    CommandLine,
    OverrideDB(PathBuf),
    ToolchainFile(PathBuf),
}

impl Display for OverrideReason {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> std::result::Result<(), fmt::Error> {
        match self {
            Self::Environment => write!(f, "environment override by RUSTUP_TOOLCHAIN"),
            Self::CommandLine => write!(f, "overridden by +toolchain on the command line"),
            Self::OverrideDB(path) => write!(f, "directory override for '{}'", path.display()),
            Self::ToolchainFile(path) => write!(f, "overridden by '{}'", path.display()),
        }
    }
}

@bestia-dev
Copy link
Contributor Author

The real command for getting the details about OverrideReason is this:
rustup show active-toolchain
result:
nightly-x86_64-unknown-linux-gnu (directory override for '/mnt/c/Users/Luciano/rustprojects/rustfmt')

@kinnison
Copy link
Contributor

kinnison commented May 8, 2020

This is looking really good. Could you please rebase to squash everything down to one or two neat commits, rather than the formatting nightmare you appear to have had in the middle, and then we should be good to merge. I'm fine with the hint, rustup show will suffice as you say.

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.

Approved modulo rebase/squash cleanup of commits and commit messages.

@bestia-dev
Copy link
Contributor Author

I made the requested changes. Is it now ok to merge?

@kinnison
Copy link
Contributor

Looks good 👍

@kinnison kinnison merged commit 4112620 into rust-lang:master May 30, 2020
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.

2 participants