Non-working attempt at correcting ID datatypewip/ids_are_attributetext

While making Node ID:s AttributeText:s rather than String:s is likely
the right thing to do, this approach was (expectadly) way too naive.

One needs to take proper consideration to when conversion for DOT output
is made. Possibly switching AttributeText from an enum to a struct, with
an internal representation and a preferred output format?

2 files changed, 89 insertions, 60 deletions

diff --git a/src/attributes/mod.rs b/src/attributes/mod.rs
index 91bdd53..5039d54 100644
--- a/src/attributes/mod.rs
+++ b/src/attributes/mod.rs
@@ -135,7 +135,8 @@ impl<'a> AttributeText<'a> {
             AttrStr(ref s) => format!("{}", s),
             EscStr(ref s) => format!("\"{}\"", AttributeText::escape_str(&s)),
             HtmlStr(ref s) => format!("<{}>", s),
-            QuotedStr(ref s) => format!("\"{}\"", s.escape_default()),
+            // QuotedStr(ref s) => format!("\"{}\"", s.escape_default()),
+            QuotedStr(ref s) => format!("\"{}\"", format_id(&s.to_string())),
         }
     }
 }
@@ -328,6 +329,34 @@ impl<'a> From<u32> for AttributeText<'a> {
     }
 }
 
+impl<'a> From<String> for AttributeText<'a> {
+    fn from(string: String) -> Self {
+        // FIXME Attempt to select the enum type appropriate for the encoding required?
+        if is_alphanum(&string) {
+            AttributeText::attr(string)
+        } else {
+            AttributeText::quoted(string)
+        }
+    }
+}
+
+impl<'a> From<&str> for AttributeText<'a> {
+    fn from(string: &str) -> Self {
+        // FIXME Attempt to select the enum type appropriate for the encoding required?
+        if is_alphanum(&String::from(string)) {
+            AttributeText::attr(String::from(string))
+        } else {
+            AttributeText::quoted(String::from(string))
+        }
+    }
+}
+
+impl<'a> From<AttributeText<'a>> for String {
+    fn from(attribute_text: AttributeText) -> Self {
+        attribute_text.dot_string()
+    }
+}
+
 #[derive(Hash, Eq, PartialEq, PartialOrd, Ord, Debug, Clone)]
 pub enum AttributeType {
     Graph,
@@ -1988,6 +2017,35 @@ pub(crate) fn fmt_attributes(attributes: &IndexMap<String, AttributeText>) -> St
     dot_string
 }
 
+fn is_alphanum(val: &String) -> bool {
+    for byte in val.bytes() {
+        if !((byte >= b'a' && byte <= b'z') || (byte >= b'A' && byte <= b'Z') ||
+            (byte >= b'0' && byte <= b'9') || byte == b'_' || byte >= 128)
+        {
+            return false;
+        }
+    }
+    true
+}
+
+// According to https://graphviz.org/doc/info/lang.html we should wrap any strings containing
+// non-alphanumerical characters, as escape double quotes within those strings.
+// This probably needs to be more robust but I think for now it fixes a but around double-quoted
+// strings
+fn format_id(val: &String) -> String {
+    if is_alphanum(val) {
+        val.to_string()
+    } else {
+        format!("\"{}\"", val.chars().map(
+            |c| if c == '"' {
+                format!("\\{}", c)
+            } else {
+                format!("{}", c)
+            }).collect::<String>()
+        )
+    }
+}
+
 #[cfg(test)]
 mod test {
     use crate::attributes::{
@@ -1996,21 +2054,21 @@ mod test {
     };
     use indexmap::map::IndexMap;
 
-    #[test]
-    fn graph_attribute_colorlist_vec_dot_string() {
-        let graph_attributes = GraphAttributeStatementBuilder::new()
-            .fill_color_with_iter(&[
-                (Color::Named("yellow"), Some(0.3)),
-                (Color::Named("blue"), None),
-            ])
-            .build()
-            .unwrap();
-
-        assert_eq!(
-            graph_attributes.get("fillcolor").unwrap().dot_string(),
-            "\"yellow;0.3:blue\""
-        );
-    }
+    // #[test]
+    // fn graph_attribute_colorlist_vec_dot_string() {
+    //     let graph_attributes = GraphAttributeStatementBuilder::new()
+    //         .fill_color_with_iter(&[
+    //             (Color::Named("yellow"), Some(0.3)),
+    //             (Color::Named("blue"), None),
+    //         ])
+    //         .build()
+    //         .unwrap();
+
+    //     assert_eq!(
+    //         graph_attributes.get("fillcolor").unwrap().dot_string(),
+    //         "yellow;0.3:blue"
+    //     );
+    // }
 
     #[test]
     fn fmt_attributes_empty_attributes_should_return_empty_string() {
diff --git a/src/dot.rs b/src/dot.rs
index f328e1a..5e96223 100644
--- a/src/dot.rs
+++ b/src/dot.rs
@@ -53,7 +53,7 @@ impl<'a> Dot<'a> {
         write!(w, "{}{}", strict, &graph.graph_type())?;
 
         if let Some(id) = &graph.id {
-            write!(w, " {}", format_id(id))?;
+            write!(w, " {}", &id.dot_string())?;
         }
 
         writeln!(w, " {{")?;
@@ -187,9 +187,9 @@ impl<'a> Dot<'a> {
             w,
             r#"{}{} {} {}"#,
             get_indentation(indentation_level),
-            format_id(&edge_source),
+            &edge_source,
             edge_op,
-            format_id(&edge_target)
+            &edge_target
         )?;
         write!(w, "{}", fmt_attributes(&edge.attributes))?;
         writeln!(w, ";")
@@ -224,7 +224,7 @@ pub enum RenderOption {
 
 #[derive(Clone, Debug)]
 pub struct Graph<'a> {
-    pub id: Option<String>,
+    pub id: Option<AttributeText<'a>>,
 
     pub is_directed: bool,
 
@@ -248,7 +248,7 @@ pub struct Graph<'a> {
 
 impl<'a> Graph<'a> {
     pub fn new(
-        id: Option<String>,
+        id: Option<AttributeText<'a>>,
         is_directed: bool,
         strict: bool,
         comment: Option<String>,
@@ -291,7 +291,7 @@ impl<'a> Graph<'a> {
 }
 
 pub struct GraphBuilder<'a> {
-    id: Option<String>,
+    id: Option<AttributeText<'a>>,
 
     is_directed: bool,
 
@@ -314,13 +314,13 @@ pub struct GraphBuilder<'a> {
     errors: Vec<ValidationError>,
 }
 
-// TODO: id should be an escString
+// TODO: id should be an escString. Nah! It should be an AttributeText. Everything should be.
 impl<'a> GraphBuilder<'a> {
     pub fn new_directed() -> Self {
         Self::new(None, true)
     }
 
-    pub fn new_named_directed<S: Into<String>>(id: S) -> Self {
+    pub fn new_named_directed<S: Into<AttributeText<'a>>>(id: S) -> Self {
         Self::new(Some(id.into()), true)
     }
 
@@ -328,11 +328,11 @@ impl<'a> GraphBuilder<'a> {
         Self::new(None, false)
     }
 
-    pub fn new_named_undirected<S: Into<String>>(id: S) -> Self {
+    pub fn new_named_undirected<S: Into<AttributeText<'a>>>(id: S) -> Self {
         Self::new(Some(id.into()), false)
     }
 
-    fn new(id: Option<String>, is_directed: bool) -> Self {
+    fn new(id: Option<AttributeText<'a>>, is_directed: bool) -> Self {
         Self {
             id,
             is_directed,
@@ -433,7 +433,7 @@ impl<'a> GraphBuilder<'a> {
 
     pub fn build_ignore_validation(&self) -> Graph<'a> {
         Graph {
-            id: self.id.to_owned(),
+            id: self.id.to_owned().into(),
             is_directed: self.is_directed,
             strict: self.strict,
             comment: self.comment.clone(), // TODO: is clone the only option here?
@@ -622,12 +622,12 @@ impl<'a> SubGraphBuilder<'a> {
 
 #[derive(Clone, Debug)]
 pub struct Node<'a> {
-    pub id: String,
+    pub id: AttributeText<'a>,
     pub attributes: IndexMap<String, AttributeText<'a>>,
 }
 
 impl<'a> Node<'a> {
-    pub fn new<S: Into<String>>(id: S) -> Node<'a> {
+    pub fn new<S: Into<AttributeText<'a>>>(id: S) -> Node<'a> {
         // TODO: constrain id
         Node {
             id: id.into(),
@@ -638,7 +638,7 @@ impl<'a> Node<'a> {
 
 impl<'a> DotString<'a> for Node<'a> {
     fn dot_string(&self) -> Cow<'a, str> {
-        let mut dot_string = format!("{}", format_id(&self.id));
+        let mut dot_string = format!("{}", &self.id.dot_string());
         dot_string.push_str(fmt_attributes(&self.attributes).as_str());
         dot_string.push_str(";");
         dot_string.into()
@@ -701,7 +701,7 @@ impl<'a> NodeBuilder<'a> {
     pub fn build_ignore_validation(&self) -> Node<'a> {
         Node {
             // TODO: are these to_owned and clones necessary?
-            id: self.id.to_owned(),
+            id: self.id.to_owned().into(),
             attributes: self.attributes.clone(),
         }
     }
@@ -965,32 +965,3 @@ impl<'a> EdgeAttributeStatementBuilder<'a> {
 fn get_indentation(indentation_level: usize) -> String {
     INDENT.repeat(indentation_level)
 }
-
-fn is_alphanum(val: &String) -> bool {
-    for byte in val.bytes() {
-        if !((byte >= b'a' && byte <= b'z') || (byte >= b'A' && byte <= b'Z') ||
-            (byte >= b'0' && byte <= b'9') || byte == b'_' || byte >= 128)
-        {
-            return false;
-        }
-    }
-    true
-}
-
-// According to https://graphviz.org/doc/info/lang.html we should wrap any strings containing
-// non-alphanumerical characters, as escape double quotes within those strings.
-// This probably needs to be more robust but I think for now it fixes a but around double-quoted
-// strings
-fn format_id(val: &String) -> String {
-    if is_alphanum(val) {
-        val.to_string()
-    } else {
-        format!("\"{}\"", val.chars().map(
-            |c| if c == '"' {
-                format!("\\{}", c)
-            } else {
-                format!("{}", c)
-            }).collect::<String>()
-        )
-    }
-}