Repository: oli-obk/cargo_metadata (rust)

Issue:
`WorkspaceMember` is not entirely correct. 
In cargo's output, package id is opaque identifier. The proper way to extract package's name, version and stuff is to look it in the `packages` array by the id and inspect the fields. `WorkspaceMember` instead [splits the id](https://docs.rs/cargo_metadata/0.6.0/src/cargo_metadata/lib.rs.html#233-250). This works at the moment, but this is not guaranteed to work.

Pull request description:
Unstringlytypify
closes #51

cc https://github.com/rust-lang/cargo/pull/6511

Patch:
diff --git a/src/lib.rs b/src/lib.rs
index 780d66d..b4f5785 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -121,6 +121,9 @@ use std::env;
 use std::path::{Path, PathBuf};
 use std::process::Command;
 use std::str::from_utf8;
+use std::fmt;
+
+use semver::Version;
 
 pub use errors::{Error, ErrorKind, Result};
 pub use dependency::{Dependency, DependencyKind};
@@ -128,34 +131,61 @@ pub use dependency::{Dependency, DependencyKind};
 mod errors;
 mod dependency;
 
+/// An "opaque" identifier for a package.
+/// It is possible to inspect the `repr` field, if the need arises, but its
+/// precise format is an implementation detail and is subject to change.
+///
+/// `Metadata` can be indexed by `PackageId`.
+#[derive(Clone, Serialize, Deserialize, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
+#[serde(transparent)]
+pub struct PackageId {
+    /// The underlying string representation of id.
+    pub repr: String
+}
+
+impl std::fmt::Display for PackageId {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        fmt::Display::fmt(&self.repr, f) }
+}
+
 #[derive(Clone, Serialize, Deserialize, Debug)]
 /// Starting point for metadata returned by `cargo metadata`
 pub struct Metadata {
     /// A list of all crates referenced by this crate (and the crate itself)
     pub packages: Vec<Package>,
     /// A list of all workspace members
-    pub workspace_members: Vec<WorkspaceMember>,
+    pub workspace_members: Vec<PackageId>,
     /// Dependencies graph
     pub resolve: Option<Resolve>,
     /// Workspace root
-    #[serde(default)]
-    pub workspace_root: String,
+    pub workspace_root: PathBuf,
     /// Build directory
-    pub target_directory: String,
+    pub target_directory: PathBuf,
     version: usize,
     #[doc(hidden)]
     #[serde(skip)]
     __do_not_match_exhaustively: (),
 }
 
+impl std::ops::Index<&PackageId> for Metadata {
+    type Output = Package;
+
+    fn index(&self, idx: &PackageId) -> &Package {
+        self.packages.iter().find(|p| p.id == *idx)
+            .unwrap_or_else(|| {
+                panic!("no package with this id: {:?}", idx)
+            })
+    }
+}
+
 #[derive(Clone, Serialize, Deserialize, Debug)]
 /// A dependency graph
 pub struct Resolve {
     /// Nodes in a dependencies graph
     pub nodes: Vec<Node>,
 
-    /// The crate for which the metadata was read
-    pub root: Option<String>,
+    /// The crate for which the metadata was read.
+    pub root: Option<PackageId>,
     #[doc(hidden)]
     #[serde(skip)]
     __do_not_match_exhaustively: (),
@@ -165,7 +195,7 @@ pub struct Resolve {
 /// A node in a dependencies graph
 pub struct Node {
     /// An opaque identifier for a package
-    pub id: String,
+    pub id: PackageId,
     /// Dependencies in a structured format.
     ///
     /// `deps` handles renamed dependencies whereas `dependencies` does not.
@@ -174,7 +204,7 @@ pub struct Node {
 
     /// List of opaque identifiers for this node's dependencies.
     /// It doesn't support renamed dependencies. See `deps`.
-    pub dependencies: Vec<String>,
+    pub dependencies: Vec<PackageId>,
 
     /// Features enabled on the crate
     #[serde(default)]
@@ -190,7 +220,7 @@ pub struct NodeDep {
     /// Crate name. If the crate was renamed, it's the new name.
     pub name: String,
     /// Package ID (opaque unique identifier)
-    pub pkg: String,
+    pub pkg: PackageId,
     #[doc(hidden)]
     #[serde(skip)]
     __do_not_match_exhaustively: (),
@@ -202,12 +232,12 @@ pub struct Package {
     /// Name as given in the `Cargo.toml`
     pub name: String,
     /// Version given in the `Cargo.toml`
-    pub version: String,
+    pub version: Version,
     /// Authors given in the `Cargo.toml`
     #[serde(default)]
     pub authors: Vec<String>,
     /// An opaque identifier for a package
-    pub id: String,
+    pub id: PackageId,
     source: Option<String>,
     /// Description as given in the `Cargo.toml`
     pub description: Option<String>,
@@ -223,7 +253,7 @@ pub struct Package {
     /// Features provided by the crate, mapped to the features required by that feature.
     pub features: HashMap<String, Vec<String>>,
     /// Path containing the `Cargo.toml`
-    pub manifest_path: String,
+    pub manifest_path: PathBuf,
     /// Categories as given in the `Cargo.toml`
     #[serde(default)]
     pub categories: Vec<String>,
@@ -290,7 +320,7 @@ pub struct Target {
     /// It doesn't apply to `lib` targets.
     pub required_features: Vec<String>,
     /// Path to the main source file of the target
-    pub src_path: String,
+    pub src_path: PathBuf,
     /// Rust edition for this target
     #[serde(default = "edition_default")]
     pub edition: String,
@@ -299,34 +329,6 @@ pub struct Target {
     __do_not_match_exhaustively: (),
 }
 
-#[derive(Clone, Debug, Serialize, Deserialize)]
-/// A workspace member. This is basically identical to `cargo::core::package_id::PackageId`, except
-/// that this does not use `Arc` internally.
-#[serde(transparent)]
-pub struct WorkspaceMember {
-    /// The raw package id as given by cargo
-    pub raw: String,
-}
-
-impl WorkspaceMember {
-    fn part(&self, n: usize) -> &str {
-        self.raw.splitn(3, ' ').nth(n).unwrap()
-    }
-    /// The name of the crate
-    pub fn name(&self) -> &str {
-        self.part(0)
-    }
-    /// The version of the crate
-    pub fn version(&self) -> semver::Version {
-        semver::Version::parse(self.part(1)).expect("bad version in cargo metadata")
-    }
-    /// The path to the crate in url format
-    pub fn url(&self) -> &str {
-        let url = self.part(2);
-        &url[1..url.len() - 1]
-    }
-}
-
 fn edition_default() -> String {
     "2015".to_string()
 }


Test patch:
diff --git a/tests/selftest.rs b/tests/selftest.rs
index 696573c..40158a9 100644
--- a/tests/selftest.rs
+++ b/tests/selftest.rs
@@ -7,7 +7,6 @@ extern crate serde_derive;
 
 use std::env::current_dir;
 use std::path::{Path, PathBuf};
-use std::env;
 
 use semver::Version;
 
@@ -39,21 +38,17 @@ fn metadata() {
     assert_eq!(metadata.packages[0].targets[1].kind[0], "test");
     assert_eq!(metadata.packages[0].targets[1].crate_types[0], "bin");
 
-    // Hack until the package metadata field reaches the stable channel (in version 1.27).
-    if env::var("TRAVIS_RUST_VERSION") != Ok("stable".into()) {
-        let package_metadata = &metadata.packages[0].metadata.as_object()
-            .expect("package.metadata must be a table. \
-            NOTE: This test currently only works on the beta and nightly channel.");
-        assert_eq!(package_metadata.len(), 1);
-
-        let value = package_metadata.get("cargo_metadata_test").unwrap();
-        let test_package_metadata: TestPackageMetadata = serde_json::from_value(value.clone())
-            .unwrap();
-        assert_eq!(test_package_metadata, TestPackageMetadata {
-            some_field: true,
-            other_field: "foo".into(),
-        });
-    }
+    let package_metadata = &metadata.packages[0].metadata.as_object()
+        .expect("package.metadata must be a table.");
+    assert_eq!(package_metadata.len(), 1);
+
+    let value = package_metadata.get("cargo_metadata_test").unwrap();
+    let test_package_metadata: TestPackageMetadata = serde_json::from_value(value.clone())
+        .unwrap();
+    assert_eq!(test_package_metadata, TestPackageMetadata {
+        some_field: true,
+        other_field: "foo".into(),
+    });
 }
 
 #[test]
@@ -96,11 +91,10 @@ fn error2() {
 #[test]
 fn metadata_deps() {
     let metadata = cargo_metadata::metadata_deps(Some(Path::new("Cargo.toml")), true).unwrap();
-    let this = metadata
-        .packages
-        .iter()
-        .find(|package| package.name == "cargo_metadata")
+    let this_id = metadata.workspace_members
+        .first()
         .expect("Did not find ourselves");
+    let this = &metadata[this_id];
 
     assert_eq!(this.name, "cargo_metadata");
     assert_eq!(this.targets.len(), 2);
@@ -125,19 +119,3 @@ fn metadata_deps() {
     assert!(serde.req.matches(&Version::parse("1.99.99").unwrap()));
     assert!(!serde.req.matches(&Version::parse("2.0.0").unwrap()));
 }
-
-#[test]
-fn workspace_member_serialization_deserialization() {
-    let original =
-        "\"security-framework 0.1.16 (registry+https://github.com/rust-lang/crates.io-index)\"";
-    let member: cargo_metadata::WorkspaceMember = serde_json::from_str(original).unwrap();
-    assert_eq!(member.name(), "security-framework");
-    assert_eq!(member.version(), Version::new(0, 1, 16));
-    assert_eq!(
-        member.url(),
-        "registry+https://github.com/rust-lang/crates.io-index"
-    );
-
-    let serialized = serde_json::to_string(&member).unwrap();
-    assert_eq!(serialized, original);
-}