Skip to content
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

Add the correct crate name (as in crates.io) to toml and notify user #98

Merged
merged 1 commit into from
Jan 3, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions src/bin/add/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ 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};

macro_rules! toml_table {
($($key:expr => $value:expr),+) => {
Expand Down Expand Up @@ -69,8 +69,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);

Expand All @@ -96,10 +95,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!()));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment why you are sure this is actually always present and unreachable is fine here?

dep.set_version(&v)
}
} else {
try!(parse_crate_name_from_uri(&self.arg_crate))
Expand Down
27 changes: 16 additions & 11 deletions src/bin/add/fetch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,35 +4,40 @@ use rustc_serialize::json;
use rustc_serialize::json::{BuilderError, Json};
use curl::{ErrCode, http};
use curl::http::handle::{Method, Request};
use cargo_edit::Manifest;
use cargo_edit::{Dependency, Manifest};
use regex::Regex;

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("CURRENT_VERSION_TEST".into()));
}

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"))
let name = try!(crate_json.find_path(&["crate", "name"])
.and_then(|n| n.as_string())
.ok_or(FetchVersionError::CratesIo(CratesIoError::NotFound)));

if name != crate_name {
println!("WARN: Added {} instead of {}", name, crate_name);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good reminder to add an actual logging facility some time soon (but not in this PR) :)

}

let version = try!(crate_json.find_path(&["crate", "max_version"])
.and_then(|v| v.as_string())
.map(|v| v.to_owned())
.ok_or(FetchVersionError::GetVersion)
.ok_or(FetchVersionError::GetVersion));

Ok(Dependency::new(name).set_version(&version))
}

quick_error! {
Expand Down
9 changes: 9 additions & 0 deletions src/dependency.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
18 changes: 18 additions & 0 deletions tests/cargo-add.rs
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,24 @@ fn adds_dependency_with_custom_target() {
}


#[test]
#[cfg(feature="test-external-apis")]
fn adds_dependency_normalized_name() {
let (_tmpdir, manifest) = clone_out_test("tests/fixtures/add/Cargo.toml.sample");

// dependency not present beforehand
let toml = get_toml(&manifest);
assert!(toml.lookup("dependencies.linked-hash-map").is_none());

assert_cli!("target/debug/cargo-add", &["add", "linked_hash_map", &format!("--manifest-path={}", manifest)] => Success,
"WARN: Added linked-hash-map instead of linked_hash_map").unwrap();

// dependency present afterwards
let toml = get_toml(&manifest);
assert!(toml.lookup("dependencies.linked-hash-map").is_some());
}


#[test]
#[should_panic]
fn fails_to_add_dependency_with_empty_target() {
Expand Down