-
Notifications
You must be signed in to change notification settings - Fork 895
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
adds --path and --nonexistent options to "rustup override unset"; adds hint to "rustup override list output" (as discussed in #642) #650
Conversation
adds hint to "rustup override list output" (as discussed in rust-lang#642)
Awesome. Thanks @Riateche ! Reviewing now. |
.after_help("If \"--path\" argument is present, removes the override toolchain for \ | ||
specified directory. If \"--nonexistent\" argument is present, removes the override \ | ||
toolchain for all nonexistent directories. Otherwise, removes the override toolchain \ | ||
for current directory.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this text to help.rs.
"toolchain for specified directory" -> "toolchain for the specified directory"
"toolchain for current directory" -> "toolchain for the current directory"
Looks great! |
This calls for several tests. Can you add tests for |
I'm a bit stuck with tests. I don't want to write fully duplicated tests for "remove" and "unset" because they should be complete aliases. But when I attempt to move test implementation to a function, it suddenly stops working, although nothing else is changed:
Output:
Next, I will need to create temporary directories to perform some tests. Is it ok to create them in the current directory or should I create them somewhere else? |
For temporary directories please use the tempdir crate. It's already a dependency of rustup, but needs to be added to The error with the test is frustrating - the output looks write. The rustup test harness is fairly primitive and can be hard to interpret. If you push the non-working commits I can take a look. |
Looks like it was just white space mismatch caused by code formatting. It seems to be working now. I'll try to write new tests. |
I've created all needed tests. I had to remove "canonicalize" call when removing nonexistent path. otherwise it produced a warning. Now Travis says there is an error on Mac OS. It may be related to "canonicalize" call because paths do not match. I don't have Mac OS so I can't fix it quickly. Maybe you can take a look. |
Looks like I got it working. Please review. |
Woo! Thanks @Riateche ! Thank you so much for sticking it out and writing those tests. I <3 tests. |
No description provided.