Refactored namespace, added test coverage, cleaned up test names

Author: Tko1

src/environment.rs

@@ -26,11 +26,11 @@ impl EnvironmentVal {
         self.curr_ns_sym.replace(name);
     }
     fn insert_into_namespace(&self, namespace: &Symbol, sym: Symbol, val: Rc<Value>) {
-        self.namespaces.insert_into_namespace(namespace, sym, val);
+        self.namespaces.insert_into_namespace(namespace, &sym, val);
     }
     fn insert_into_current_namespace(&self, sym: Symbol, val: Rc<Value>) {
         self.namespaces
-            .insert_into_namespace(&*self.curr_ns_sym.borrow(), sym, val);
+            .insert_into_namespace(&*self.curr_ns_sym.borrow(), &sym, val);
     }
     fn get_from_namespace(&self, namespace: &Symbol, sym: &Symbol) -> Rc<Value> {
         self.namespaces.get(namespace, sym)
@@ -44,9 +44,8 @@ impl EnvironmentVal {
     /// Default main environment
     fn new_main_val() -> EnvironmentVal {
         let curr_ns_sym = Symbol::intern("user");
-        let curr_ns = Namespace::from_sym(curr_ns_sym.clone());
-        let namespaces = Namespaces(RefCell::new(HashMap::new()));
-        namespaces.insert(curr_ns_sym.clone(), curr_ns);
+        let namespaces = Namespaces::new();
+        namespaces.create_namespace(&curr_ns_sym);
         EnvironmentVal {
             curr_ns_sym: RefCell::new(curr_ns_sym),
             namespaces,

src/namespace.rs

@@ -10,17 +10,17 @@ pub struct Namespace {
     mappings: RefCell<HashMap<Symbol, Rc<Value>>>,
 }
 impl Namespace {
-    pub fn new(name: Symbol, mappings: RefCell<HashMap<Symbol, Rc<Value>>>) -> Namespace {
-        Namespace { name, mappings }
+    pub fn new(name: &Symbol, mappings: RefCell<HashMap<Symbol, Rc<Value>>>) -> Namespace {
+        Namespace { name: name.unqualified(), mappings }
     }
-    pub fn from_sym(name: Symbol) -> Namespace {
+    pub fn from_sym(name: &Symbol) -> Namespace {
         Namespace::new(name, RefCell::new(HashMap::new()))
     }
-    pub fn insert(&self, sym: Symbol, val: Rc<Value>) {
-        self.mappings.borrow_mut().insert(sym, val);
+    pub fn insert(&self, sym: &Symbol, val: Rc<Value>) {
+        self.mappings.borrow_mut().insert(sym.unqualified(), val);
     }
     pub fn get(&self, sym: &Symbol) -> Rc<Value> {
-        match self.mappings.borrow_mut().get(sym) {
+        match self.mappings.borrow_mut().get(&sym.unqualified()) {
             Some(val) => Rc::clone(val),
             None => Rc::new(Value::Condition(format!("1 Undefined symbol {}", sym.name))),
         }
@@ -33,13 +33,17 @@ impl Namespaces {
     pub fn new() -> Namespaces {
         Namespaces(RefCell::new(HashMap::new()))
     }
-    /// Insert a new namespace of name (sym)
-    pub fn insert(&self, sym: Symbol, namespace: Namespace) {
+    fn insert(&self, namespace: Namespace) {
         // When storing / retrieving from namespaces, we want
         // namespace unqualified keys
-        let sym = sym.unqualified();
-        self.0.borrow_mut().insert(sym, namespace);
+        self.0.borrow_mut().insert(namespace.name.unqualified(), namespace);
     }
+    /// Adds a new namespace to internal HashMap (but does
+    /// *not* return a Namespace or reference to one)
+    pub fn create_namespace(&self,sym: &Symbol){
+        self.insert(Namespace::from_sym(sym));
+    }
+    /// Insert a new namespace of name (sym)
     pub fn has_namespace(&self, namespace_sym: &Symbol) -> bool {
         let namespace_sym = namespace_sym.unqualified();
 
@@ -50,32 +54,28 @@ impl Namespaces {
             None => false,
         }
     }
-    // @TODO Consider writing `sym` as reference here, because we clone it anyways
-    //       Only reason to keep it is `inserts` are pass-by-value here normally,
-    //       since the idea normally is you are literally inserting the keys too
-    //       I'd prefer that consistency, unless we find it has a noticeable
-    //       performance impact
     /// Insert a binding (sym = val) *into* namespace (namespace)
-    pub fn insert_into_namespace(&self, namespace_sym: &Symbol, sym: Symbol, val: Rc<Value>) {
+    /// If namespace doesn't exist, create it 
+    pub fn insert_into_namespace(&self, namespace_sym: &Symbol, sym: &Symbol, val: Rc<Value>) {
         let mut namespace_sym = &namespace_sym.unqualified();
         // We will only use this if ns isn't ""
         let symbol_namespace_sym = Symbol::intern(&sym.ns);
 
-        if sym.ns != "" {
+        if sym.has_ns() {
             namespace_sym = &symbol_namespace_sym;
         }
 
         let namespaces = self.0.borrow();
         let namespace = namespaces.get(namespace_sym);
         match namespace {
             Some(namespace) => {
-                namespace.insert(sym.unqualified(), val);
+                namespace.insert(sym, val);
             }
             None => {
                 drop(namespaces);
-                let namespace = Namespace::from_sym(namespace_sym.clone());
-                namespace.insert(sym.unqualified(), val);
-                self.insert(namespace_sym.unqualified(), namespace);
+                let namespace = Namespace::from_sym(namespace_sym);
+                namespace.insert(sym, val);
+                self.insert(namespace);
             }
         }
     }
@@ -87,7 +87,7 @@ impl Namespaces {
 
         // @TODO just make it an Optional<String>
         // If our sym is namespace qualified,  use that as our namespace
-        if sym.ns != "" {
+        if sym.has_ns() {
             namespace_sym = Symbol::intern(&sym.ns);
         }
 
@@ -105,13 +105,145 @@ impl Namespaces {
 
 #[cfg(test)]
 mod tests {
+    // 'Struct' here because its not immediately clear why, when testing this,
+    // why the word 'namespace' is repeated and that this is actually specifically
+    // a struct 
+    mod namespace_struct {
+        use crate::namespace::Namespace;
+        use crate::symbol::Symbol;
+        use crate::value::Value;
+        use std::rc::Rc;
+        use std::cell::RefCell;
+        use std::collections::HashMap;
+
+        #[test]
+        fn new() {
+            let namespace = Namespace::new(&Symbol::intern("a"),RefCell::new(HashMap::new()));
+            assert_eq!(namespace.name, Symbol::intern("a"));
+            assert!(namespace.mappings.borrow().is_empty());
+        }
+
+        #[test]
+        fn new_removes_namespace_from_qualified_symbol() {
+            let namespace = Namespace::new(&Symbol::intern_with_ns("ns","a"),RefCell::new(HashMap::new()));
+            assert_eq!(namespace.name, Symbol::intern("a"));
+            assert!(namespace.name != Symbol::intern_with_ns("ns","a"));
+            assert!(namespace.mappings.borrow().is_empty());
+        }
+        #[test]
+        fn new_namespace_starts_empty() {
+            let namespace = Namespace::new(&Symbol::intern("a"),RefCell::new(HashMap::new()));
+            let namespace2 = Namespace::new(&Symbol::intern_with_ns("ns","b"),RefCell::new(HashMap::new()));
+            assert!(namespace.mappings.borrow().is_empty());
+            assert!(namespace2.mappings.borrow().is_empty());
+        }
+
+        #[test]
+        fn from_sym() {
+            let namespace = Namespace::from_sym(&Symbol::intern_with_ns("ns","name"));
+            assert_eq!(namespace.name,Symbol::intern("name"));
+            assert!(namespace.name != Symbol::intern_with_ns("ns","name"));
+            assert!(namespace.mappings.borrow().is_empty());
+        }
+        #[test]
+        fn insert() {
+            let namespace = Namespace::from_sym(&Symbol::intern("name"));
+            namespace.insert(&Symbol::intern("a"),Rc::new(Value::Nil));
+            namespace.insert(&Symbol::intern_with_ns("ns","b"),Rc::new(Value::Nil));
+            assert_eq!(namespace.name,Symbol::intern("name"));
+            assert!(namespace.name != Symbol::intern("ns"));
+            assert!(namespace.name != Symbol::intern_with_ns("ns","name"));
+
+            namespace.insert(&Symbol::intern("c"),Rc::new(Value::Nil));
+            match &*namespace.get(&Symbol::intern("c")) {
+                Value::Condition(_) => panic!("We are unable to get a symbol we've put into our namespace created with from_sym()"),
+                _ => {}
+            }
+        }
+
+        #[test]
+        fn get() {
+            let namespace = Namespace::from_sym(&Symbol::intern("name"));
+            namespace.insert(&Symbol::intern("a"),Rc::new(Value::Nil));
+            namespace.insert(&Symbol::intern_with_ns("ns","b"),Rc::new(Value::Nil));
+            match &*namespace.get(&Symbol::intern("a")) {
+                Value::Condition(_) => panic!("We are unable to get a symbol we've put into our namespace created with from_sym()"),
+                _ => {}
+            }
 
-    mod namespaces {
+            match &*namespace.get(&Symbol::intern("b")) {
+                Value::Condition(_) => panic!("We are unable to get a symbol we've put into our namespace created with from_sym()"),
+                _ => {}
+            }
+
+            match &*namespace.get(&Symbol::intern("ns")) {
+                Value::Condition(_) => {}
+                _ => panic!("We are able to get a symbol whose name is the namespace of another symbol we inserted (and note, that namesapce should be dropped altogether on insert)"),
+            }
+
+            match &*namespace.get(&Symbol::intern("sassafrass")) {
+                Value::Condition(_) => {}
+                _ => panic!("We are able to get a symbol we didn't insert without a Condition being thrown"),
+            }
+
+            match &*namespace.get(&Symbol::intern_with_ns("ns","b")) {
+                Value::Condition(_) => panic!("We are unable to get a symbol by trying to get a namespace qualified version of it (the namespace normally should be irrelevant and automatically drop)"),
+                _ => {}
+            }
+
+            match &*namespace.get(&Symbol::intern_with_ns("chicken","a")) {
+                Value::Condition(_) => panic!("We are unable to get a symbol by trying to get a namespace qualified (with a random namespace) version of it (the namespace normally should be irrelevant and automatically drop)"),
+                _ => {}
+            }
+        }
+    }
+    mod namespaces_newtype {
+        use crate::namespace::Namespace;
         use crate::namespace::Namespaces;
         use crate::symbol::Symbol;
         use crate::value::Value;
         use std::rc::Rc;
+        fn new() {
+            let namespaces = Namespaces::new();
+            assert!(namespaces.0.borrow().is_empty());
+        }
+        fn insert() {
+            let namespaces = Namespaces::new();
+            let namespace  = Namespace::from_sym(&Symbol::intern("clojure.core"));
+            namespace.insert(&Symbol::intern("+"),Rc::new(Value::Nil));
+            // Namespace should be dropped; doesn't matter when inserting into
+            // a namespace
+            namespace.insert(&Symbol::intern_with_ns("clojure.math","+"),Rc::new(Value::Nil));
+            /////////////////////////////////////////////////////////////
+            namespaces.insert(namespace);
 
+            assert_eq!(Value::Nil,*namespaces.get(&Symbol::intern("clojure.core"),&Symbol::intern("+")));
+            assert_eq!(Value::Nil,*namespaces.get(&Symbol::intern_with_ns("ns-doesn't-matter","clojure.core"),&Symbol::intern("+")));
+            assert_eq!(Value::Nil,*namespaces.get(&Symbol::intern_with_ns("ns-doesn't-matter","clojure.core"),&Symbol::intern_with_ns("ns-still-doesn't-matter","+")));
+            //Ie, we should get a Condition, because there is no clojure.math/+ 
+            assert!(Value::Nil != *namespaces.get(&Symbol::intern("clojure.math"),&Symbol::intern("+")));
+        }
+        fn has_namespace() {
+            let namespaces = Namespaces::new();
+            let namespace  = Namespace::from_sym(&Symbol::intern("clojure.core"));
+            namespace.insert(&Symbol::intern("+"),Rc::new(Value::Nil));
+
+            assert!(namespaces.has_namespace(&Symbol::intern("clojure.core")));
+            assert!(namespaces.has_namespace(&Symbol::intern_with_ns("ns-doesn't-matter","clojure.core")));
+            assert!(!namespaces.has_namespace(&Symbol::intern("+")));
+            // Note; ns-doesn't-matter *isn't* the namespace this time 
+            assert!(!namespaces.has_namespace(&Symbol::intern_with_ns("clojure.core","ns-doesn't-matter")));
+        }
+        fn insert_into_namespace() {
+            let namespaces = Namespaces::new();
+            namespaces.insert_into_namespace(&Symbol::intern("clojure.core"), &Symbol::intern("+"), Rc::new(Value::Nil));
+            assert!(!namespaces.has_namespace(&Symbol::intern("random_ns")));
+            assert!(namespaces.has_namespace(&Symbol::intern("clojure.core")));
+
+            assert_eq!(Value::Nil, *namespaces.get(&Symbol::intern("clojure.core"),&Symbol::intern("+")));
+            assert!(Value::Nil != *namespaces.get(&Symbol::intern("clojure.core"),&Symbol::intern("other-sym")));
+            assert!(Value::Nil != *namespaces.get(&Symbol::intern("other-ns"),&Symbol::intern("+")));
+        }
         ////////////////////////////////////////////////////////////////////////////////////////////////////
         //
         //  pub fn get(&self,namespace_sym: &Symbol,sym: &Symbol) -> Rc<Value>
@@ -139,7 +271,7 @@ mod tests {
             let clojure_core1_plus_1 = Symbol::intern("clojure.core1/+1");
             namespaces.insert_into_namespace(
                 &Symbol::intern("clojure.core1"),
-                Symbol::intern("+1"),
+                &Symbol::intern("+1"),
                 Rc::new(Value::Nil),
             );
             match &*namespaces.get(&Symbol::intern("clojure.your"), &clojure_core1_plus_1) {
@@ -162,7 +294,7 @@ mod tests {
             let clojure_core_plus = Symbol::intern("clojure.core/+");
             namespaces.insert_into_namespace(
                 &Symbol::intern("clojure.core"),
-                Symbol::intern("+"),
+                &Symbol::intern("+"),
                 Rc::new(Value::Nil),
             );
             // Really means get +/+,  but is overwritten to mean get clojure.core/+
@@ -185,7 +317,7 @@ mod tests {
             let plus_2 = Symbol::intern("+2");
             namespaces.insert_into_namespace(
                 &Symbol::intern("core2"),
-                Symbol::intern("+2"),
+                &Symbol::intern("+2"),
                 Rc::new(Value::Nil),
             );
             // Get intern("core2/+2")
@@ -208,7 +340,7 @@ mod tests {
             let namespaces = Namespaces::new();
             namespaces.insert_into_namespace(
                 &Symbol::intern("core2"),
-                Symbol::intern("+2"),
+                &Symbol::intern("+2"),
                 Rc::new(Value::Nil),
             );