Print information about updated packages on cargo update. #1621
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @wycats (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see CONTRIBUTING.md for more information. |
| match dependencies.get(dep.name()) { | ||
| Some(&version) => { | ||
| if version != dep.version() { | ||
| println!(" Updating {} v{} -> v{}", dep.name(), dep.version(), version) |
There was a problem hiding this comment.
Could this thread through the Shell from UpdateOptions instead of printing directly to stdout?
|
Nice, thanks! Could you also add some tests for the other messages printed like "Removed" and "New dependency"? |
|
I will use the Shell and add tests. I have to refactor my code since it returns different results on different runs due to the unordered nature of HashMap 😄 |
|
Adressed all comments by @alexcrichton |
| } else {Ok(())} | ||
| } | ||
| (None, None) => unreachable!(), | ||
| }); |
There was a problem hiding this comment.
This may be better structured like:
let (status, msg) = match dep {
(None, Some(pkg)) => ("Adding", format!(...)),
(Some(pkg), None) => ("Removing", format!(...)),
(Some(pkg1), Some(pkg2)) => {
if pkg1.version() == pkg2.version() { continue }
("Updating", format!(...))
}
(None, None) => continue,
};
try!(opts.config.shell().status(...))
|
Looking great! I added some comments about different sources and multiple versions, and it would be nice to have exhaustive tests but it's ok to skimp a little bit. It would, however, be nice to see at least a test for multiple versions in an existing graph being updated to one version in a later graph (just to see what the output does). |
|
In talking with @wycats, he also brings up that the dependency graph can change as a result of a modification of |
I also would like to see an actual output screenshot for these kinds of significant changes 😄 |
|
It seems that this has stalled since opening, so I'm going to close, but feel free to reopen with comments addressed! |
Compare #1621. Like that pull request, this one solves #984. I have rewritten most of @pyfisch's logic to account for @alexcrichton's comments. Specifically, this code will correctly handle adding/removing multiple versions of one package, as well as several packages with different sources, but the same name. The output looks like this: ``` [georgev@desertvoice cargo]$ cargo update Updating registry `https://github.com/rust-lang/crates.io-index` Updating libc v0.1.8 -> v0.1.10 Updating memchr v0.1.3 -> v0.1.5 Updating num v0.1.26 -> v0.1.27 Updating rand v0.3.9 -> v0.3.10 Updating rustc-serialize v0.3.15 -> v0.3.16 ``` Comments welcome. r? @alexcrichton
Solves #984