add command suggestion #3817
add command suggestion #3817
Conversation
|
@rami3l that's the overall idea, still not super sure about the suggestion logic. At first I was thinking to pattern matching only for a specific use case but then changed to possibly extend to how many case are necessary by inserting a new entry inside the hashmap, what do you think? |
| @@ -97,7 +100,11 @@ pub fn main() -> Result<utils::ExitCode> { | |||
| write!(process().stdout().lock(), "{e}")?; | |||
| return Ok(utils::ExitCode(0)); | |||
| } | |||
| Err(e) => return Err(e.into()), | |||
| Err(e) => { | |||
There was a problem hiding this comment.
I believe that's not the right place to add the handling logic for rustup update. Have you actually tested the code that you added?
Please have a look at the rustup update handling logic here, and find out where that error was thrown, catch it, print another message using the info!() macro if that is the expected type of error, and finally re-throw it (you may adjust the ordering of the error message and the new one if that makes the implementation simpler).
After that, please add a regression test in an appropriate spot (like here) to ensure the resolution of the issue.
| @@ -125,3 +132,20 @@ pub fn main() -> Result<utils::ExitCode> { | |||
|
|
|||
| self_update::install(no_prompt, verbose, quiet, opts) | |||
| } | |||
|
|
|||
| fn handle_suggestion() -> Result<()> { | |||
| let mut suggestions = HashMap::new(); | |||
There was a problem hiding this comment.
IMHO using a HashMap has added no value to the current implementation. There's no need to "plan ahead" in this particular situation. If a HashMap turns out to be required, it can be added in another patch in the future, but not right now.
| "note: use `rustup self update` to update rustup itself", | ||
| ); | ||
|
|
||
| let args: Vec<String> = std::env::args().skip(1).collect(); |
There was a problem hiding this comment.
Using std::env::args() is not appropriate here, see #3817 (comment). Instead, you should extract all necessary info from the m: &ArgMatches parameter.
| @@ -1,3 +1,5 @@ | |||
| use std::collections::HashMap; | |||
There was a problem hiding this comment.
Finally, at Rustup it is recommended to use atomic commits with informative commit messages, please have a look at the examples on the master branch. This means that you'll have to do git rebase -i for quite a bunch of times to get the right commits (and the right messages).
Closes #3812.