Refactor combine (#35)

Dunklas · Jul 8, 2022 · da80ee5 · da80ee5
1 parent 94421bc
commit da80ee5
Show file tree

Hide file tree

Showing 6 changed files with 78 additions and 63 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "souper"
-version = "0.4.1"
+version = "0.4.2"
 edition = "2021"
 
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

diff --git a/src/main.rs b/src/main.rs
@@ -62,7 +62,7 @@ fn main() {
         .map(|meta_key| (meta_key, json!("")))
         .collect::<Map<String, Value>>();
 
-    let current_contexts = match output_path.is_file() {
+    let mut current_contexts = match output_path.is_file() {
         true => match SoupContexts::from_output_file(&output_path) {
             Ok(contexts) => contexts,
             Err(e) => {
@@ -84,8 +84,8 @@ fn main() {
         }
     };
 
-    let combined_contexts = SoupContexts::combine(current_contexts, scanned_contexts);
-    if let Err(e) = combined_contexts.write_to_file(&output_path) {
+    current_contexts.combine(&scanned_contexts);
+    if let Err(e) = current_contexts.write_to_file(&output_path) {
         panic!("{}", e);
     }
 }
diff --git a/src/soup/contexts_combine.rs b/src/soup/contexts_combine.rs
@@ -1,39 +1,33 @@
-use std::collections::{BTreeSet, HashMap};
 use serde_json::{Map, Value};
 use crate::soup::model::{Soup, SoupContexts};
 
 impl SoupContexts {
-    pub fn combine(base: SoupContexts, other: SoupContexts) -> SoupContexts {
-        let mut result_contexts = base.contexts.clone();
-        other.contexts.iter()
-            .filter(|(path, _soups)| !base.contexts.contains_key(*path))
-            .for_each(|(path, soups)| {
-                result_contexts.insert(path.clone(), soups.clone());
-            });
-        base.contexts.iter().for_each(|(path, soups)| {
-            if !other.contexts.contains_key(path) {
-                result_contexts.remove(path);
-                return;
+    pub fn combine(&mut self, other: &SoupContexts) {
+        for (path, soups) in other.contexts() {
+            if !self.contexts.contains_key(path) {
+                self.contexts.insert(path.clone(), soups.clone());
             }
-            let meta_by_name = soups.iter()
-                .map(|soup| (&soup.name, &soup.meta))
-                .collect::<HashMap<&String, &Map<String, Value>>>();
-            let other_soups = other.contexts.get(path).unwrap();
-            let mut desired_soups = other_soups.iter()
-                .map(|soup| Soup{
-                    name: soup.name.clone(),
-                    version: soup.version.clone(),
-                    meta: match meta_by_name.get(&soup.name) {
-                        Some(meta) => combine_meta(*meta, &soup.meta),
-                        None => soup.meta.clone(),
+        }
+        self.contexts.retain(|path,_| other.contexts().contains_key(path));
+
+        for (path, other_soups) in other.contexts() {
+            let self_soups = self.contexts.get_mut(path).unwrap();
+
+            for other_soup in other_soups {
+                let soup = Soup {
+                    name: other_soup.name.clone(),
+                    version: other_soup.version.clone(),
+                    meta: match self_soups.iter().find(|x| x.name == other_soup.name) {
+                        Some(soup) => combine_meta(&soup.meta, &other_soup.meta),
+                        None => other_soup.meta.clone()
                     }
-                })
-                .collect::<BTreeSet<Soup>>();
-            let target_soups = result_contexts.get_mut(path).unwrap();
-            target_soups.clear();
-            target_soups.append(&mut desired_soups);
-        });
-        SoupContexts { contexts: result_contexts }
+                };
+                self_soups.remove(&soup);
+                self_soups.insert(soup);
+            }
+
+            self_soups.retain(|soup| other_soups.contains(soup));
+        }
     }
 }
 
@@ -76,71 +70,89 @@ mod tests {
 
     #[test]
     fn combine_add_context() {
-        let base = empty_contexts();
+        let mut base = empty_contexts();
         let other = create_contexts("src/package.json", vec![
             Soup { name: "some-dep".to_owned(), version: "1.0.0".to_owned(), meta: meta(vec![]) }
         ]);
 
-        let result = SoupContexts::combine(base, other);
-        assert_eq!(1, result.contexts.len());
-        assert_eq!(true, result.contexts.contains_key("src/package.json"));
+        base.combine(&other);
+        assert_eq!(1, base.contexts.len());
+        assert_eq!(true, base.contexts.contains_key("src/package.json"));
     }
 
     #[test]
     fn combine_remove_context() {
-        let base = create_contexts("src/package.json", vec![
+        let mut base = create_contexts("src/package.json", vec![
             Soup { name: "some-dep".to_owned(), version: "1.0.0".to_owned(), meta: meta(vec![]) }
         ]);
         let other = empty_contexts();
 
-        let result = SoupContexts::combine(base, other);
-        assert_eq!(true, result.contexts.is_empty());
+        base.combine(&other);
+        assert_eq!(true, base.contexts.is_empty());
     }
 
     #[test]
     fn combine_added_soup() {
-        let base = create_contexts("src/package.json", vec![
+        let mut base = create_contexts("src/package.json", vec![
             Soup { name: "some-dep".to_owned(), version: "1.0.0".to_owned(), meta: meta(vec![]) }
         ]);
         let other = create_contexts("src/package.json", vec![
             Soup { name: "some-dep".to_owned(), version: "1.0.0".to_owned(), meta: meta(vec![]) },
             Soup { name: "some-other-dep".to_owned(), version: "1.0.0".to_owned(), meta: meta(vec![]) }
         ]);
 
-        let result = SoupContexts::combine(base, other);
-        assert_eq!(1, result.contexts.len());
-        let soups = result.contexts.get("src/package.json").unwrap();
+        base.combine(&other);
+        assert_eq!(1, base.contexts.len());
+        let soups = base.contexts.get("src/package.json").unwrap();
         assert_eq!(2, soups.len());
     }
 
     #[test]
     fn combine_removed_soup() {
-        let base = create_contexts("src/package.json", vec![
+        let mut base = create_contexts("src/package.json", vec![
             Soup { name: "some-dep".to_owned(), version: "1.0.0".to_owned(), meta: meta(vec![]) },
             Soup { name: "some-other-dep".to_owned(), version: "1.0.0".to_owned(), meta: meta(vec![]) }
         ]);
         let other = create_contexts("src/package.json", vec![
             Soup { name: "some-dep".to_owned(), version: "1.0.0".to_owned(), meta: meta(vec![]) }
         ]);
 
-        let result = SoupContexts::combine(base, other);
-        assert_eq!(1, result.contexts.len());
-        let soups = result.contexts.get("src/package.json").unwrap();
+        base.combine(&other);
+        assert_eq!(1, base.contexts.len());
+        let soups = base.contexts.get("src/package.json").unwrap();
         assert_eq!(1, soups.len());
     }
 
+    #[test]
+    fn no_update_preserves_meta() {
+        let mut base = create_contexts("src/package.json", vec![
+            Soup { name: "some-dep".to_owned(), version: "1.0.0".to_owned(), meta: meta(vec![("some-meta", "some-value")]) }
+        ]);
+        let other = create_contexts("src/package.json", vec![
+            Soup { name: "some-dep".to_owned(), version: "1.0.0".to_owned(), meta: meta(vec![]) }
+        ]);
+
+        base.combine(&other);
+        assert_eq!(1, base.contexts.len());
+        let soups = base.contexts.get("src/package.json").unwrap();
+        let soup = soups.iter().find(|s| s.name == "some-dep").unwrap();
+        assert_eq!("some-dep", soup.name);
+        assert_eq!("1.0.0", soup.version);
+        assert_eq!(meta(vec![("some-meta", "some-value")]), soup.meta);
+    }
+
     #[test]
     fn update_soup_version_preserves_meta() {
-        let base = create_contexts("src/package.json", vec![
+        let mut base = create_contexts("src/package.json", vec![
             Soup { name: "some-dep".to_owned(), version: "1.0.0".to_owned(), meta: meta(vec![("some-meta", "some-value")]) }
         ]);
         let other = create_contexts("src/package.json", vec![
             Soup { name: "some-dep".to_owned(), version: "1.2.0".to_owned(), meta: meta(vec![]) }
         ]);
 
-        let result = SoupContexts::combine(base, other);
-        assert_eq!(1, result.contexts.len());
-        let soups = result.contexts.get("src/package.json").unwrap();
+        base.combine(&other);
+        assert_eq!(1, base.contexts.len());
+        let soups = base.contexts.get("src/package.json").unwrap();
         let soup = soups.iter().find(|s| s.name == "some-dep").unwrap();
         assert_eq!("some-dep", soup.name);
         assert_eq!("1.2.0", soup.version);
@@ -149,15 +161,15 @@ mod tests {
 
     #[test]
     fn append_meta_from_other() {
-        let base = create_contexts("src/package.json", vec![
+        let mut base = create_contexts("src/package.json", vec![
             Soup { name: "some-dep".to_owned(), version: "1.0.0".to_owned(), meta: meta(vec![("some-meta", "some-value")]) }
         ]);
         let other = create_contexts("src/package.json", vec![
             Soup { name: "some-dep".to_owned(), version: "1.0.0".to_owned(), meta: meta(vec![("requirements", "")]) }
         ]);
-        let result = SoupContexts::combine(base, other);
-        assert_eq!(1, result.contexts.len());
-        let soups = result.contexts.get("src/package.json").unwrap();
+        base.combine(&other);
+        assert_eq!(1, base.contexts.len());
+        let soups = base.contexts.get("src/package.json").unwrap();
         let soup = soups.iter().find(|s| s.name == "some-dep").unwrap();
         assert_eq!("some-dep", soup.name);
         assert_eq!("1.0.0", soup.version);
@@ -169,16 +181,16 @@ mod tests {
 
     #[test]
     fn append_meta_from_other_no_overwrite() {
-        let base = create_contexts("src/package.json", vec![
+        let mut base = create_contexts("src/package.json", vec![
             Soup { name: "some-dep".to_owned(), version: "1.0.0".to_owned(), meta: meta(vec![("requirements", "a-requirement")]) }
         ]);
         let other = create_contexts("src/package.json", vec![
             Soup { name: "some-dep".to_owned(), version: "1.0.0".to_owned(), meta: meta(vec![("requirements", "")]) }
         ]);
 
-        let result = SoupContexts::combine(base, other);
-        assert_eq!(1, result.contexts.len());
-        let soups = result.contexts.get("src/package.json").unwrap();
+        base.combine(&other);
+        assert_eq!(1, base.contexts.len());
+        let soups = base.contexts.get("src/package.json").unwrap();
         let soup = soups.iter().find(|s| s.name == "some-dep").unwrap();
         assert_eq!("some-dep", soup.name);
         assert_eq!("1.0.0", soup.version);

diff --git a/src/soup/contexts_io.rs b/src/soup/contexts_io.rs
@@ -108,7 +108,7 @@ impl SoupContexts {
                 message: format!("Not able to create file: {} ({})", file_path.as_ref().display(), e)
             })
         };
-        let json = match serde_json::to_string_pretty(&self.contexts) {
+        let json = match serde_json::to_string_pretty(&self.contexts()) {
             Ok(json) => json,
             Err(e) => return Err(SouperIoError{
                 message: format!("Not able to serialize to json: {}", e)

diff --git a/src/soup/model.rs b/src/soup/model.rs
@@ -51,6 +51,9 @@ impl SoupContexts {
     pub fn empty() -> SoupContexts {
         SoupContexts { contexts: BTreeMap::new() }
     }
+    pub fn contexts(&self) -> &BTreeMap<String, BTreeSet<Soup>> {
+        &self.contexts
+    }
 }
 
 pub struct SouperIoError {