Merge pull request geiger-rs#188 from jmcconnell26/NOISSUE-CleanupErr…

…orHandling

NOISSUE - Clean up error handling, remove unwrap() calls, logging

cargo-geiger/src/format/display.rs

@@ -2,8 +2,7 @@ use crate::format::pattern::Pattern;
 use crate::format::Chunk;
 use crate::mapping::{
     CargoMetadataParameters, GetLicenceFromCargoMetadataPackageId,
-    GetPackageNameFromCargoMetadataPackageId,
-    GetPackageVersionFromCargoMetadataPackageId,
+    GetPackageNameAndVersionFromCargoMetadataPackageId,
     GetRepositoryFromCargoMetadataPackageId,
 };
 
@@ -32,22 +31,13 @@ impl<'a> fmt::Display for Display<'a> {
                     }
                 }
                 Chunk::Package => {
-                    (write!(
-                        fmt,
-                        "{} {}",
-                        self.cargo_metadata_parameters
-                            .krates
-                            .get_package_name_from_cargo_metadata_package_id(
-                                self.package
-                            )
-                            .unwrap(),
-                        self.cargo_metadata_parameters
-                            .krates
-                            .get_package_version_from_cargo_metadata_package_id(
-                                self.package
-                            )
-                            .unwrap()
-                    ))?
+                    if let Some((package_name, package_version)) = self.cargo_metadata_parameters
+                        .krates
+                        .get_package_name_and_version_from_cargo_metadata_package_id(self.package) {
+                        (write!(fmt, "{} {}", package_name, package_version))?
+                    } else {
+                        eprintln!("Failed to format Package: {}", self.package)
+                    }
                 }
                 Chunk::Raw(ref s) => (fmt.write_str(s))?,
                 Chunk::Repository => {

cargo-geiger/src/format/table/handle_text_tree_line.rs

@@ -22,14 +22,15 @@ pub fn handle_text_tree_line_extra_deps_group(
     table_lines: &mut Vec<String>,
     tree_vines: String,
 ) {
-    let name = get_kind_group_name(dep_kind);
-    if name.is_none() {
-        return;
+    if let Some(name) = get_kind_group_name(dep_kind) {
+        // TODO: Fix the alignment on macOS (others too?)
+        table_lines.push(format!(
+            "{}{}{}",
+            table_row_empty(),
+            tree_vines,
+            name
+        ));
     }
-    let name = name.unwrap();
-
-    // TODO: Fix the alignment on macOS (others too?)
-    table_lines.push(format!("{}{}{}", table_row_empty(), tree_vines, name));
 }
 
 pub fn handle_text_tree_line_package(

cargo-geiger/src/graph.rs

@@ -8,7 +8,7 @@ use cargo::core::Workspace;
 use cargo::util::interning::InternedString;
 use cargo::util::CargoResult;
 use cargo::Config;
-use cargo_metadata::{DependencyKind, PackageId};
+use cargo_metadata::{Dependency, DependencyKind, Package, PackageId};
 use cargo_platform::Cfg;
 use petgraph::graph::NodeIndex;
 use std::collections::hash_map::Entry;
@@ -124,49 +124,39 @@ fn add_package_dependencies_to_graph(
     pending_packages: &mut Vec<PackageId>,
 ) {
     let index = graph.nodes[&package_id];
-    let package = cargo_metadata_parameters
-        .krates
-        .node_for_kid(&package_id)
-        .unwrap()
-        .krate
-        .clone();
 
-    for (dependency_package_id, _) in cargo_metadata_parameters
+    let krates_node_option =
+        cargo_metadata_parameters.krates.node_for_kid(&package_id);
+
+    let dep_not_replaced_option = cargo_metadata_parameters
         .metadata
-        .deps_not_replaced(package_id)
-    {
-        let dependency_iterator = package
-            .dependencies
-            .iter()
-            .filter(|d| {
-                d.matches_ignoring_source(
-                    cargo_metadata_parameters.krates,
-                    dependency_package_id.clone(),
-                )
-            })
-            .filter(|d| graph_configuration.extra_deps.allows(d.kind))
-            .filter(|d| {
-                d.target
-                    .as_ref()
-                    .and_then(|p| {
-                        graph_configuration.target.map(|t| {
-                            match graph_configuration.cfgs {
-                                None => false,
-                                Some(cfgs) => p.matches(t, cfgs),
-                            }
-                        })
-                    })
-                    .unwrap_or(true)
-            });
+        .deps_not_replaced(package_id.clone());
 
-        for dependency in dependency_iterator {
-            add_graph_node_if_not_present_and_edge(
-                dependency,
-                dependency_package_id.clone(),
-                graph,
-                index,
-                pending_packages,
-            );
+    match (krates_node_option, dep_not_replaced_option) {
+        (Some(krates_node), Some(dependencies)) => {
+            let package = krates_node.krate.clone();
+
+            for (dependency_package_id, _) in dependencies {
+                let dependency_iterator = filter_dependencies(
+                    cargo_metadata_parameters,
+                    &dependency_package_id,
+                    graph_configuration,
+                    &package,
+                );
+
+                for dependency in dependency_iterator {
+                    add_graph_node_if_not_present_and_edge(
+                        dependency,
+                        dependency_package_id.clone(),
+                        graph,
+                        index,
+                        pending_packages,
+                    );
+                }
+            }
+        }
+        _ => {
+            eprintln!("Failed to add package dependencies to graph for Package Id: {}", package_id)
         }
     }
 }
@@ -195,6 +185,39 @@ fn build_graph_prerequisites<'a>(
     (extra_deps, target)
 }
 
+fn filter_dependencies<'a>(
+    cargo_metadata_parameters: &'a CargoMetadataParameters,
+    dependency_package_id: &'a PackageId,
+    graph_configuration: &'a GraphConfiguration,
+    package: &'a Package,
+) -> Vec<&'a Dependency> {
+    package
+        .dependencies
+        .iter()
+        .filter(|d| {
+            d.matches_ignoring_source(
+                cargo_metadata_parameters.krates,
+                dependency_package_id.clone(),
+            )
+            .unwrap_or(false)
+        })
+        .filter(|d| graph_configuration.extra_deps.allows(d.kind))
+        .filter(|d| {
+            d.target
+                .as_ref()
+                .and_then(|p| {
+                    graph_configuration.target.map(
+                        |t| match graph_configuration.cfgs {
+                            None => false,
+                            Some(cfgs) => p.matches(t, cfgs),
+                        },
+                    )
+                })
+                .unwrap_or(true)
+        })
+        .collect::<Vec<&Dependency>>()
+}
+
 #[cfg(test)]
 mod graph_tests {
     use super::*;

cargo-geiger/src/mapping.rs

@@ -19,10 +19,12 @@ pub trait DepsNotReplaced {
     fn deps_not_replaced(
         &self,
         package_id: cargo_metadata::PackageId,
-    ) -> Vec<(
-        cargo_metadata::PackageId,
-        HashSet<cargo_metadata::Dependency>,
-    )>;
+    ) -> Option<
+        Vec<(
+            cargo_metadata::PackageId,
+            HashSet<cargo_metadata::Dependency>,
+        )>,
+    >;
 }
 
 pub trait GetLicenceFromCargoMetadataPackageId {
@@ -32,18 +34,11 @@ pub trait GetLicenceFromCargoMetadataPackageId {
     ) -> Option<String>;
 }
 
-pub trait GetPackageNameFromCargoMetadataPackageId {
-    fn get_package_name_from_cargo_metadata_package_id(
+pub trait GetPackageNameAndVersionFromCargoMetadataPackageId {
+    fn get_package_name_and_version_from_cargo_metadata_package_id(
         &self,
         package_id: &cargo_metadata::PackageId,
-    ) -> Option<String>;
-}
-
-pub trait GetPackageVersionFromCargoMetadataPackageId {
-    fn get_package_version_from_cargo_metadata_package_id(
-        &self,
-        package_id: &cargo_metadata::PackageId,
-    ) -> Option<cargo_metadata::Version>;
+    ) -> Option<(String, cargo_metadata::Version)>;
 }
 
 pub trait GetRepositoryFromCargoMetadataPackageId {
@@ -54,15 +49,15 @@ pub trait GetRepositoryFromCargoMetadataPackageId {
 }
 
 pub trait GetRoot {
-    fn get_root(&self) -> PathBuf;
+    fn get_root(&self) -> Option<PathBuf>;
 }
 
 pub trait MatchesIgnoringSource {
     fn matches_ignoring_source(
         &self,
         krates: &Krates,
         package_id: cargo_metadata::PackageId,
-    ) -> bool;
+    ) -> Option<bool>;
 }
 
 pub trait QueryResolve {
@@ -76,14 +71,14 @@ pub trait ToCargoCoreDepKind {
 pub trait ToCargoGeigerDependencyKind {
     fn to_cargo_geiger_dependency_kind(
         &self,
-    ) -> cargo_geiger_serde::DependencyKind;
+    ) -> Option<cargo_geiger_serde::DependencyKind>;
 }
 
 pub trait ToCargoGeigerPackageId {
     fn to_cargo_geiger_package_id(
         &self,
         metadata: &Metadata,
-    ) -> cargo_geiger_serde::PackageId;
+    ) -> Option<cargo_geiger_serde::PackageId>;
 }
 
 pub trait ToCargoGeigerSource {

cargo-geiger/src/mapping/krates.rs

@@ -1,7 +1,6 @@
 use crate::mapping::{
     GetLicenceFromCargoMetadataPackageId,
-    GetPackageNameFromCargoMetadataPackageId,
-    GetPackageVersionFromCargoMetadataPackageId,
+    GetPackageNameAndVersionFromCargoMetadataPackageId,
     GetRepositoryFromCargoMetadataPackageId, QueryResolve,
 };
 
@@ -18,23 +17,14 @@ impl GetLicenceFromCargoMetadataPackageId for Krates {
     }
 }
 
-impl GetPackageNameFromCargoMetadataPackageId for Krates {
-    fn get_package_name_from_cargo_metadata_package_id(
+impl GetPackageNameAndVersionFromCargoMetadataPackageId for Krates {
+    fn get_package_name_and_version_from_cargo_metadata_package_id(
         &self,
         package_id: &cargo_metadata::PackageId,
-    ) -> Option<String> {
-        self.node_for_kid(package_id)
-            .map(|package| package.krate.clone().name)
-    }
-}
-
-impl GetPackageVersionFromCargoMetadataPackageId for Krates {
-    fn get_package_version_from_cargo_metadata_package_id(
-        &self,
-        package_id: &cargo_metadata::PackageId,
-    ) -> Option<cargo_metadata::Version> {
-        self.node_for_kid(package_id)
-            .map(|package| package.krate.clone().version)
+    ) -> Option<(String, cargo_metadata::Version)> {
+        self.node_for_kid(package_id).map(|package| {
+            (package.krate.clone().name, package.krate.clone().version)
+        })
     }
 }
 
@@ -50,12 +40,18 @@ impl GetRepositoryFromCargoMetadataPackageId for Krates {
 
 impl QueryResolve for Krates {
     fn query_resolve(&self, query: &str) -> Option<cargo_metadata::PackageId> {
-        let package_spec = PkgSpec::from_str(query).unwrap();
-        self.krates_by_name(package_spec.name.as_str())
-            .filter(|(_, node)| package_spec.matches(&node.krate))
-            .map(|(_, node)| node.krate.clone().id)
-            .collect::<Vec<cargo_metadata::PackageId>>()
-            .pop()
+        match PkgSpec::from_str(query) {
+            Ok(package_spec) => self
+                .krates_by_name(package_spec.name.as_str())
+                .filter(|(_, node)| package_spec.matches(&node.krate))
+                .map(|(_, node)| node.krate.clone().id)
+                .collect::<Vec<cargo_metadata::PackageId>>()
+                .pop(),
+            _ => {
+                eprintln!("Failed to construct PkgSpec from string: {}", query);
+                None
+            }
+        }
     }
 }
 
@@ -82,8 +78,10 @@ mod krates_tests {
     fn get_package_name_from_cargo_metadata_package_id_test() {
         let (krates, metadata) = construct_krates_and_metadata();
         let package = metadata.root_package().unwrap();
-        let package_name = krates
-            .get_package_name_from_cargo_metadata_package_id(&package.id)
+        let (package_name, _) = krates
+            .get_package_name_and_version_from_cargo_metadata_package_id(
+                &package.id,
+            )
             .unwrap();
         assert_eq!(package_name, package.name);
     }
@@ -92,8 +90,10 @@ mod krates_tests {
     fn get_package_version_from_cargo_metadata_package_id_test() {
         let (krates, metadata) = construct_krates_and_metadata();
         let package = metadata.root_package().unwrap();
-        let package_version = krates
-            .get_package_version_from_cargo_metadata_package_id(&package.id)
+        let (_, package_version) = krates
+            .get_package_name_and_version_from_cargo_metadata_package_id(
+                &package.id,
+            )
             .unwrap();
         assert_eq!(package_version, package.version);
     }
@@ -147,19 +147,15 @@ mod krates_tests {
         let (krates, _) = construct_krates_and_metadata();
         let package_id = krates.query_resolve(input_query_string).unwrap();
 
-        assert_eq!(
-            krates
-                .get_package_name_from_cargo_metadata_package_id(&package_id)
-                .unwrap(),
-            expected_package_name
-        );
+        let (package_name, package_version) = krates
+            .get_package_name_and_version_from_cargo_metadata_package_id(
+                &package_id,
+            )
+            .unwrap();
 
-        assert_eq!(
-            krates
-                .get_package_version_from_cargo_metadata_package_id(&package_id)
-                .unwrap(),
-            expected_package_version
-        );
+        assert_eq!(package_name, expected_package_name);
+
+        assert_eq!(package_version, expected_package_version);
     }
 
     fn construct_krates_and_metadata() -> (Krates, Metadata) {