Merge pull request #141 from ocornoc/graysons-suggestions

Graysons suggestions

Author: EricCrosson

src/lerna_manifest.rs

@@ -90,11 +90,11 @@ impl LernaManifest {
     }
 
     pub fn into_package_manifests_by_package_name(
-        mut self,
+        self,
     ) -> Result<HashMap<String, PackageManifest>, Box<dyn Error>> {
         self.internal_package_manifests
-            .drain(0..)
-            .map(|manifest| Ok((manifest.contents.name.to_owned(), manifest)))
+            .into_iter()
+            .map(|manifest| Ok((manifest.contents.name.clone(), manifest)))
             .collect()
     }
 }

src/link.rs

@@ -40,11 +40,11 @@ fn create_project_references(children: &[String]) -> TypescriptParentProjectRefe
     let mut sorted_children = children.to_owned();
     sorted_children.sort_unstable();
     TypescriptParentProjectReference {
-        files: [].to_vec(),
+        files: Vec::new(),
         references: sorted_children
-            .iter()
-            .map(|child| TypescriptProjectReference {
-                path: child.to_string(),
+            .into_iter()
+            .map(|path| TypescriptProjectReference {
+                path,
             })
             .collect(),
     }
@@ -118,11 +118,10 @@ fn link_package_dependencies(
             let package_directory = package_manifest.directory();
             let mut tsconfig = TypescriptConfig::from_directory(&opts.root, &package_directory)?;
             let internal_dependencies =
-                package_manifest.get_internal_dependencies(&package_manifest_by_package_name);
+                package_manifest.internal_dependencies_iter(&package_manifest_by_package_name);
 
             let desired_project_references: Vec<TypescriptProjectReference> = {
                 let mut typescript_project_references: Vec<String> = internal_dependencies
-                    .iter()
                     .map(|dependency| {
                         diff_paths(dependency.directory(), package_manifest.directory())
                             .expect(

src/package_manifest.rs

@@ -88,58 +88,55 @@ impl AsRef<PackageManifest> for PackageManifest {
 }
 
 impl PackageManifest {
-    pub fn get_internal_dependencies<'a, T>(
+    pub fn internal_dependencies_iter<'a, T>(
         &'a self,
-        package_manifests_by_package_name: &HashMap<String, &'a T>,
-    ) -> Vec<&'a PackageManifest>
+        package_manifests_by_package_name: &'a HashMap<String, &'a T>,
+    ) -> impl Iterator<Item = &'a PackageManifest>
     where
         T: AsRef<PackageManifest>,
     {
-        let get_dependency_group = |dependency_group: &str| -> Vec<&'a String> {
-            self.contents
-                .extra_fields
-                .get(dependency_group)
-                .and_then(serde_json::Value::as_object)
-                .map(|object| object.keys().collect())
-                .unwrap_or_default()
-        };
-
-        get_dependency_group("dependencies")
+        static DEPENDENCY_GROUPS: &[&str] = &[
+            "dependencies",
+            "devDependencies",
+            "optionalDependencies",
+            "peerDependencies",
+        ];
+
+        DEPENDENCY_GROUPS
             .iter()
-            .chain(get_dependency_group("devDependencies").iter())
-            .chain(get_dependency_group("optionalDependencies").iter())
-            .chain(get_dependency_group("peerDependencies").iter())
+            // only iterate over the objects corresponding to each dependency group
+            .filter_map(|dependency_group| {
+                self.contents.extra_fields.get(dependency_group)?.as_object()
+            })
+            // get all dependency names from all groups
+            .flat_map(|dependency_group_value| dependency_group_value.keys())
             // filter out external packages
             .filter_map(|package_name| {
                 package_manifests_by_package_name
-                    .get(*package_name)
-                    .cloned()
-                    .map(|thing| thing.as_ref())
+                    .get(package_name)
+                    .map(|&thing| thing.as_ref())
             })
-            .collect()
     }
 
     pub fn transitive_internal_dependency_package_names<'a>(
         &self,
         package_manifest_by_package_name: &HashMap<String, &'a PackageManifest>,
     ) -> Vec<&'a PackageManifest> {
         // Depth-first search all transitive internal dependencies of package
-        let mut seen_package_names: HashSet<&str> = HashSet::new();
-        let mut internal_dependencies: HashSet<String> = HashSet::new();
-        let mut to_visit_package_manifests: VecDeque<&PackageManifest> = VecDeque::new();
+        let mut seen_package_names = HashSet::new();
+        let mut internal_dependencies = HashSet::new();
+        let mut to_visit_package_manifests = VecDeque::new();
 
         to_visit_package_manifests.push_back(self);
 
-        while !to_visit_package_manifests.is_empty() {
-            let current_manifest = to_visit_package_manifests.pop_front().unwrap();
+        while let Some(current_manifest) = to_visit_package_manifests.pop_front() {
             seen_package_names.insert(&current_manifest.contents.name);
 
             for dependency in current_manifest
-                .get_internal_dependencies(package_manifest_by_package_name)
-                .iter()
+                .internal_dependencies_iter(package_manifest_by_package_name)
             {
                 internal_dependencies.insert(dependency.contents.name.to_owned());
-                if !seen_package_names.contains(&dependency.contents.name.as_ref()) {
+                if !seen_package_names.contains(&dependency.contents.name) {
                     to_visit_package_manifests.push_back(dependency);
                 }
             }
@@ -156,10 +153,10 @@ impl PackageManifest {
             .collect()
     }
 
-    pub fn get_dependency_group_mut<'a>(
-        &'a mut self,
+    pub fn get_dependency_group_mut(
+        &mut self,
         group: &DependencyGroup,
-    ) -> Option<&'a mut serde_json::Map<String, serde_json::Value>> {
+    ) -> Option<&mut serde_json::Map<String, serde_json::Value>> {
         let group_index = match group {
             DependencyGroup::Dependencies => "dependencies",
             DependencyGroup::DevDependencies => "devDependencies",