feat: add hint to run rustup self when err desc is self
#3901
Conversation
src/dist/mod.rs
Outdated
| let desc_string = desc.to_string(); | ||
| if desc_string.as_str() == "self" { | ||
| info!("If you meant to modify rustup itself, try run `rustup self`"); | ||
| } | ||
| Err(RustupError::InvalidToolchainName(desc_string).into()) |
There was a problem hiding this comment.
I think it would be better to do this near the top of the method? We can just check for desc == "self" and return the error immediately, which would save a bunch of work trying to parse.
There was a problem hiding this comment.
@djc My suggestion is that since FromStr is used in many places other than update and uninstall (such as install), this will cause many false positives, and instead we should modify the value_parser for these two commands only.
There was a problem hiding this comment.
But self is still invalid in all those places, right -- worst case this will provide a clearer error message?
There was a problem hiding this comment.
@djc I'm afraid that will make the error message less clear. rustup install self doesn't make sense and if the info leads the user to rustup self they won't find anything related to install there.
There was a problem hiding this comment.
Sure, but users are presumably also much less likely to invoke rustup install self. I still think it would be better to embed this check inside the FromStr impl where it is implicitly used in any future code that wants to consume toolchain names instead of remembering that we have to add an extra check everywhere, we can make the error message slightly more generic to make sure it doesn't lose clarity.
There was a problem hiding this comment.
@djc I still feel uneasy about adding side effects in conversions like FromStr... @ChrisDenton What do you think?
There was a problem hiding this comment.
I would argue that logging is pretty much "unobservable" as side effects go, so while I agree that in general side effects in something like a FromStr implementations could be a bit iffy, I think logging is an exception to this.
Would you have problems with adding logging to other kinds of "pure" functions?
There was a problem hiding this comment.
@djc Actually... it's not about purity, it's about the fact that conversions can be used in various places across the codebase, many of which are not easily detectable via call hierarchy graph rust-analyzer gives us, and I'm afraid of this potentially adding a log line somewhere unexpected. Since the very nature of this feature is to add a hint, I guess it'd be better to get the hint only when we are explicitly expecting for it.
For example, rustup uninstall uses ResolvableToolchainName, and its conversions relies on the conversions defined for PartialToolchainDesc first, and that of CustomToolchainName as a fallback. Let's say we're still using a743b3a, and it so happens that there's a custom toolchain named self, then the first conversion will definitely fail. However, before the fallback happens, the log line is already printed out.
There was a problem hiding this comment.
Thanks! IMHO there are two things left to do:
- Squash all the feature changes together, as
rustupencourages atomic commits, and the current changes are seemingly small enough to be squash into a single commit. - In a new commit, add some regression tests in
tests/suite/cli_misc.rsto prevent future regressions.
Fine, I might do it later cuz I'm now in a holiday. |
|
@rami3l Can I close this PR and open a new one? |
@Xerxes-2 AFAIK merge commits will be automatically dropped when doing an interactive rebase. See https://stackoverflow.com/a/4784082. |
Use original error instead of rewriting avoid duplicate `.to_string()` remove unnecessary `.as_str()`
Xerxes-2
force-pushed
the
smart-guess-update-self
branch
from
9844106 to
4689949
Compare
|
@Xerxes-2 Thanks again for helping out! |
Closes #3812.