Auto merge of #101 - killercup:feature/new-year-new-bugfixes, r=kille…

…rcup

New year, new bugfixes

This is a rollup of #98 (which replaces #78), #99, and #100.

@sebasgarcep and @sinkuu, thank you for your PRs! Do you have time to review this? I've add a few tests and changed to code slightly, before running Rustfmt and Clippy.

Just to make sure Github gets all the references:

Fixes #52
Fixes #88
Fixes #92

src/bin/add/args.rs

@@ -1,9 +1,10 @@
 //! Handle `cargo add` arguments
-use semver;
-use std::error::Error;
+
 use cargo_edit::Dependency;
 use fetch::{get_crate_name_from_github, get_crate_name_from_gitlab, get_crate_name_from_path,
-            get_latest_version};
+            get_latest_dependency};
+use semver;
+use std::error::Error;
 
 macro_rules! toml_table {
     ($($key:expr => $value:expr),+) => {
@@ -69,8 +70,7 @@ impl Args {
                 let le_crate = if crate_name_has_version(arg_crate) {
                         try!(parse_crate_name_with_version(arg_crate))
                     } else {
-                        let v = try!(get_latest_version(&self.arg_crate));
-                        Dependency::new(arg_crate).set_version(&v)
+                        try!(get_latest_dependency(arg_crate))
                     }
                     .set_optional(self.flag_optional);
 
@@ -96,10 +96,13 @@ impl Args {
                 } else if let Some(ref path) = self.flag_path {
                     dependency.set_path(path)
                 } else {
+                    let dep = try!(get_latest_dependency(&self.arg_crate));
                     let v = format!("{prefix}{version}",
                                     prefix = self.get_upgrade_prefix().unwrap_or(""),
-                                    version = try!(get_latest_version(&self.arg_crate)));
-                    dependency.set_version(&v)
+                                    // if version is unavailable
+                                    // `get_latest_dependency` must have returned `Err(FetchVersionError::GetVersion)`
+                                    version = dep.version().unwrap_or_else(|| unreachable!()));
+                    dep.set_version(&v)
                 }
             } else {
                 try!(parse_crate_name_from_uri(&self.arg_crate))

src/bin/add/fetch.rs

@@ -1,38 +1,109 @@
-use std::env;
-use std::path::Path;
-use rustc_serialize::json;
-use rustc_serialize::json::{BuilderError, Json};
+
+use cargo_edit::{Dependency, Manifest};
 use curl::{ErrCode, http};
 use curl::http::handle::{Method, Request};
-use cargo_edit::Manifest;
 use regex::Regex;
+use rustc_serialize::json;
+use rustc_serialize::json::{BuilderError, Json};
+use std::env;
+use std::path::Path;
 
 const REGISTRY_HOST: &'static str = "https://crates.io";
 
 /// Query latest version from crates.io
 ///
-/// The latest version will be returned as a string. This will fail, when
+/// The latest version will be returned as a `Dependency`. This will fail, when
 ///
 /// - there is no Internet connection,
 /// - the response from crates.io is an error or in an incorrect format,
 /// - or when a crate with the given name does not exist on crates.io.
-pub fn get_latest_version(crate_name: &str) -> Result<String, FetchVersionError> {
+pub fn get_latest_dependency(crate_name: &str) -> Result<Dependency, FetchVersionError> {
     if env::var("CARGO_IS_TEST").is_ok() {
         // We are in a simulated reality. Nothing is real here.
         // FIXME: Use actual test handling code.
-        return Ok("CURRENT_VERSION_TEST".into());
+        return Ok(Dependency::new(crate_name)
+            .set_version(&format!("{}--CURRENT_VERSION_TEST", crate_name)));
     }
 
     let crate_data = try!(fetch_cratesio(&format!("/crates/{}", crate_name)));
     let crate_json = try!(Json::from_str(&crate_data));
 
-    crate_json.as_object()
-        .and_then(|c| c.get("crate"))
-        .and_then(|c| c.as_object())
-        .and_then(|c| c.get("max_version"))
-        .and_then(|v| v.as_string())
-        .map(|v| v.to_owned())
-        .ok_or(FetchVersionError::GetVersion)
+    let dep = try!(read_latest_version(crate_json));
+
+    if dep.name != crate_name {
+        println!("WARN: Added `{}` instead of `{}`", dep.name, crate_name);
+    }
+
+    Ok(dep)
+}
+
+/// Read latest version from JSON structure
+///
+/// Assumes the version are sorted so that the first non-yanked version is the
+/// latest, and thus the one we want.
+fn read_latest_version(crate_json: Json) -> Result<Dependency, FetchVersionError> {
+    let versions = try!(crate_json.as_object()
+        .and_then(|c| c.get("versions"))
+        .and_then(Json::as_array)
+        .ok_or(FetchVersionError::GetVersion));
+
+    let latest = try!(versions.iter()
+        .filter_map(Json::as_object)
+        .find(|&v| !v.get("yanked").and_then(Json::as_boolean).unwrap_or(true))
+        .ok_or(FetchVersionError::AllYanked));
+
+    let name = try!(latest.get("crate")
+        .and_then(Json::as_string)
+        .map(String::from)
+        .ok_or(FetchVersionError::CratesIo(CratesIoError::NotFound)));
+
+    let version = try!(latest.get("num")
+        .and_then(Json::as_string)
+        .map(String::from)
+        .ok_or(FetchVersionError::GetVersion));
+
+    Ok(Dependency::new(&name).set_version(&version))
+}
+
+#[test]
+fn get_latest_version_from_json_test() {
+    let json = Json::from_str(r#"{
+      "versions": [
+        {
+          "crate": "treexml",
+          "num": "0.3.1",
+          "yanked": true
+        },
+        {
+          "crate": "treexml",
+          "num": "0.3.0",
+          "yanked": false
+        }
+      ]
+    }"#).unwrap();
+
+    assert_eq!(read_latest_version(json).unwrap().version().unwrap(),
+               "0.3.0");
+}
+
+#[test]
+fn get_no_latest_version_from_json_when_all_are_yanked() {
+    let json = Json::from_str(r#"{
+      "versions": [
+        {
+          "crate": "treexml",
+          "num": "0.3.1",
+          "yanked": true
+        },
+        {
+          "crate": "treexml",
+          "num": "0.3.0",
+          "yanked": true
+        }
+      ]
+    }"#).unwrap();
+
+    assert!(read_latest_version(json).is_err());
 }
 
 quick_error! {
@@ -51,6 +122,7 @@ quick_error! {
             cause(err)
         }
         GetVersion { description("get version error") }
+        AllYanked { description("No non-yanked version found") }
     }
 }
 

src/bin/add/main.rs

@@ -14,8 +14,8 @@ extern crate curl;
 extern crate quick_error;
 
 use std::error::Error;
-use std::process;
 use std::io::{self, Write};
+use std::process;
 
 extern crate cargo_edit;
 use cargo_edit::Manifest;

src/bin/list/list.rs

@@ -1,8 +1,9 @@
-use pad::{Alignment, PadStr};
-use toml;
+
 
 use cargo_edit::Manifest;
 use list_error::ListError;
+use pad::{Alignment, PadStr};
+use toml;
 
 /// List the dependencies for manifest section
 #[allow(deprecated)] // connect -> join
@@ -41,11 +42,7 @@ pub fn list_section(manifest: &Manifest, section: &str) -> Result<String, ListEr
                             name =
                                 name.pad_to_width_with_alignment(name_max_len, Alignment::Left),
                             version = version,
-                            optional = if optional {
-                                " (optional)"
-                            } else {
-                                ""
-                            }));
+                            optional = if optional { " (optional)" } else { "" }));
     }
 
     Ok(output.connect("\n"))

src/bin/list/main.rs

@@ -13,8 +13,8 @@ extern crate toml;
 extern crate quick_error;
 
 use std::error::Error;
-use std::process;
 use std::io::{self, Write};
+use std::process;
 
 extern crate cargo_edit;
 use cargo_edit::Manifest;

src/bin/list/tree.rs

@@ -1,9 +1,10 @@
-use std::collections::BTreeMap;
 
-use toml;
 
 use cargo_edit::Manifest;
 use list_error::ListError;
+use std::collections::BTreeMap;
+
+use toml;
 
 pub type PkgName = String;
 pub type PkgVersion = String;
@@ -97,20 +98,12 @@ fn list_deps_helper(pkgs: &Packages,
         // don't want
         // to print out a branch.
         for is_last in levels.iter().cloned() {
-            let indent = if is_last {
-                "    "
-            } else {
-                "│   "
-            };
+            let indent = if is_last { "    " } else { "│   " };
             output.push_str(indent);
         }
 
         let is_last = i == deps.len() - 1;
-        let branch = if is_last {
-            "└──"
-        } else {
-            "├──"
-        };
+        let branch = if is_last { "└──" } else { "├──" };
 
         output.push_str(&format!("{} {} ({})\n", branch, dep.0, dep.1));
 

src/bin/rm/main.rs

@@ -11,8 +11,8 @@ extern crate semver;
 extern crate rustc_serialize;
 
 use std::error::Error;
-use std::process;
 use std::io::{self, Write};
+use std::process;
 
 extern crate cargo_edit;
 use cargo_edit::Manifest;

src/dependency.rs

@@ -67,6 +67,15 @@ impl Dependency {
         self
     }
 
+    /// Get version of dependency
+    pub fn version(&self) -> Option<&str> {
+        if let DependencySource::Version(ref version) = self.source {
+            Some(version)
+        } else {
+            None
+        }
+    }
+
     /// Convert dependency to TOML
     ///
     /// Returns a tuple with the dependency's name and either the version as a String or the

src/manifest.rs

@@ -1,14 +1,13 @@
+use dependency::Dependency;
+use std::{env, str};
 use std::collections::BTreeMap;
 use std::collections::btree_map::Entry;
-use std::{env, str};
 use std::error::Error;
 use std::fs::{self, File, OpenOptions};
 use std::io::{Read, Write};
 use std::path::{Path, PathBuf};
 use toml;
 
-use dependency::Dependency;
-
 /// Enumeration of errors which can occur when working with a rust manifest.
 quick_error! {
     #[derive(Debug)]
@@ -166,7 +165,7 @@ impl Manifest {
                              dep: &Dependency)
                              -> Result<(), ManifestError> {
         let (ref name, ref data) = dep.to_toml();
-        let ref mut manifest = self.data;
+        let manifest = &mut self.data;
 
         let mut entry = manifest;
         for part in table {
@@ -203,7 +202,7 @@ impl Manifest {
     /// ```
     #[cfg_attr(feature = "dev", allow(toplevel_ref_arg))]
     pub fn remove_from_table(&mut self, table: &str, name: &str) -> Result<(), ManifestError> {
-        let ref mut manifest = self.data;
+        let manifest = &mut self.data;
         let entry = manifest.entry(String::from(table));
 
         match entry {
@@ -264,8 +263,8 @@ fn format_parse_error(parser: &toml::Parser) -> Option<ManifestError> {
 
 #[cfg(test)]
 mod tests {
-    use super::*;
     use dependency::Dependency;
+    use super::*;
     use toml;
 
     #[test]