Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upadds --path and --nonexistent options to "rustup override unset"; adds hint to "rustup override list output" (as discussed in #642) #650
Conversation
Riateche
added some commits
Aug 11, 2016
This comment has been minimized.
This comment has been minimized.
|
Awesome. Thanks @Riateche ! Reviewing now. |
brson
reviewed
Aug 11, 2016
| .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.") |
This comment has been minimized.
This comment has been minimized.
brson
Aug 11, 2016
Contributor
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"
brson
reviewed
Aug 11, 2016
| } | ||
| ).collect()))); | ||
| if list.is_empty() { | ||
| println!("No nonexistent paths detected."); |
This comment has been minimized.
This comment has been minimized.
brson
reviewed
Aug 11, 2016
| } else { | ||
| println!("Overrides:"); | ||
| let mut any_not_exist = false; |
This comment has been minimized.
This comment has been minimized.
brson
Aug 11, 2016
Contributor
I'd prefer not to make these UI changes at this time, to keep some consistency with toolchain list, and because there are already a variety of inconsistencies and issues with the UI that I'd prefer to tackle more holistically later. Can you keep the empty message "no overrides"; remove the "Overrides:" header; add a single blank println!("") before the "you may remove overrides ..." hint?
This comment has been minimized.
This comment has been minimized.
|
Looks great! |
This comment has been minimized.
This comment has been minimized.
|
This calls for several tests. Can you add tests for |
This comment has been minimized.
This comment has been minimized.
|
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? |
This comment has been minimized.
This comment has been minimized.
|
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. |
This comment has been minimized.
This comment has been minimized.
|
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. |
Riateche
added some commits
Aug 13, 2016
This comment has been minimized.
This comment has been minimized.
|
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. |
This comment has been minimized.
This comment has been minimized.
|
Looks like I got it working. Please review. |
brson
merged commit d0c3063
into
rust-lang:master
Aug 18, 2016
This comment has been minimized.
This comment has been minimized.
|
Woo! Thanks @Riateche ! Thank you so much for sticking it out and writing those tests. I <3 tests. |
Riateche commentedAug 11, 2016
No description provided.