Fixing failing silently (#28)

* Added errors and updated tests for the following cases:
 - removing edges between nodes which do not exist.
 - removing attributes which have not been set.

* 1. Added quotations arround node names in errors.
2. When removing edges between nodes in a graph if no edges exist and
   error is outputted.

* - Added function to check if a label is valid.
- Reduced some code duplication in other spots
TODO: add label validity checking to all functions
where labels may be added.

* added planning

* added attribute removal when setting an existing attribute on graphs, added more tests and went through operations which could fail and reasonably should error

* Delete plan.md

Author: mpan322

gap/dot.gd

@@ -161,6 +161,8 @@ DeclareOperation("GraphvizAddNode", [IsGraphvizGraphDigraphOrContext, IsObject])
 #! If no nodes with the same name are in the graph then the edge's nodes will be
 #! added to the graph. If different nodes with the same name are in the graph
 #! then the operation fails.
+#! TODO I dont believe this is accurate - think it will connect existing ones
+#! underlying private function would fail though - TODO double check.
 DeclareOperation("GraphvizAddEdge",
 [IsGraphvizGraphDigraphOrContext, IsObject, IsObject]);
 

gap/dot.gi

@@ -281,8 +281,9 @@ function(x, name, value)
          " be removed using GraphvizRemoveAttr");
   fi;
 
-  attrs := GraphvizAttrs(x);
   name := String(name);
+  GV_RemoveGraphAttrIfExists(x, name);
+  attrs := GraphvizAttrs(x);
   value := String(value);
   if ' ' in value then
     # Replace with call to GV_QuoteName or whatever TODO
@@ -297,9 +298,31 @@ end);
 InstallMethod(GraphvizSetAttr, "for a graphviz (di)graph or context and object",
 [IsGraphvizGraphDigraphOrContext, IsObject],
 function(x, value)
-  local attrs;
-  attrs := GraphvizAttrs(x);
+  local attrs, match, pred;
+
+  match := function(lookup, target)
+      local idx, pred;
+      idx := 1;
 
+      pred := function(i)
+          return i <= Length(target) and i <= Length(lookup)
+                 and lookup[i] = target[i] and lookup[i] <> '=';
+      end;
+
+      while pred(idx) do
+        idx := idx + 1;
+      od;
+      if idx > Length(lookup) or idx > Length(lookup) then
+        return false;
+      elif lookup[idx] = '=' and target[idx] = '=' then
+        return true;
+      fi;
+      return false;
+  end;
+
+  attrs := GraphvizAttrs(x);
+  x!.Attrs := Filtered(attrs, attr -> not match(attr, value));
+  attrs := GraphvizAttrs(x);
   Add(attrs, String(value));
   return x;
 end);
@@ -464,7 +487,6 @@ function(g, name)
   if nodes[name] <> fail then
     Unbind(nodes[name]);
   else
-    # Don't just silently do nothing
     ErrorFormatted("the 2nd argument (node name string) \"{}\"",
                    " is not a node of the 1st argument (a graphviz",
                    " (di)graph/context)",
@@ -511,6 +533,20 @@ InstallMethod(GraphvizRemoveEdges,
 "for a graphviz (di)graph or context, string, and string",
 [IsGraphvizGraphDigraphOrContext, IsString, IsString],
 function(g, hn, tn)
+  local lh, lt, len;
+
+  # if no such nodes exist -> error out
+  lh := GV_FindNode(g, hn) = fail;
+  lt := GV_FindNode(g, tn) = fail;
+  if lh and lt then
+    ErrorFormatted("no nodes with names \"{}\" or \"{}\"", hn, tn);
+  elif lh then
+    ErrorFormatted("no node with name \"{}\"", hn);
+  elif lt then
+    ErrorFormatted("no node with name \"{}\"", tn);
+  fi;
+
+  len := Length(GraphvizEdges(g));
   GraphvizFilterEdges(g,
     function(e)
       local head, tail, tmp;
@@ -523,6 +559,10 @@ function(g, hn, tn)
         return tmp and (hn <> GraphvizName(tail) or tn <> GraphvizName(head));
       fi;
     end);
+  if len - Length(GraphvizEdges(g)) = 0 then
+    ErrorFormatted("no edges exist from \"{}\" to \"{}\"",
+                   tn, hn);
+  fi;
 
   return g;
 end);
@@ -537,44 +577,33 @@ InstallMethod(GraphvizRemoveAttr, "for a graphviz object and an object",
 function(obj, attr)
   local attrs;
   attrs := GraphvizAttrs(obj);
-  # TODO error if no such attr?
-  Unbind(attrs[String(attr)]);
+  attr  := String(attr);
+
+  if not IsBound(attrs[attr]) then
+    ErrorFormatted("the 2nd argument (attribute name) \"{}\" ",
+                   "is not set on the provided object.",
+                   attr);
+  fi;
+
+  Unbind(attrs[attr]);
   return obj;
 end);
 
-# TODO this doesn't currently work as intended, see:
-# https://github.com/digraphs/graphviz/issues/23
 InstallMethod(GraphvizRemoveAttr,
 "for a graphviz (di)graph or context and an object",
 [IsGraphvizGraphDigraphOrContext, IsObject],
 function(obj, attr)
-  local attrs, i, match;
+  local attrs, len;
   attrs := GraphvizAttrs(obj);
-  attr  := String(attr);
-
-  # checks if they attribute names match the one being removed
-  match := function(key, str)
-    for i in [1 .. Length(key)] do
-      if i > Length(str) or key[i] <> str[i] then
-        return false;
-      fi;
-    od;
-
-    i := i + 1;
-    while i <= Length(str) do
-      if str[i] = '=' then
-        return true;
-      elif str[i] <> '\s' and str[i] <> '\t' then
-        return false;
-      fi;
-      i := i + 1;
-    od;
-
-    # attributes which are not key value or removal by value
-    return true;
-  end;
-
-  obj!.Attrs := Filtered(attrs, s -> not match(attr, s));
+  len := Length(attrs);
+
+  GV_RemoveGraphAttrIfExists(obj, attr);
+  # error if no attributes were removed i.e. did not exist
+  if Length(obj!.Attrs) - len = 0 then
+    ErrorFormatted("the 2nd argument (attribute name or attribute) \"{}\" ",
+                   "is not set on the provided object.",
+                   attr);
+  fi;
   return obj;
 end);
 
@@ -608,9 +637,10 @@ function(gv, labels)
                    "has incorrect length, expected {}, but ",
                    "found {}", Size(GraphvizNodes(gv)), Size(labels));
   fi;
-  # TODO GV_ErrorIfNotValidLabel
+
   nodes := GraphvizNodes(gv);
   for i in [1 .. Size(nodes)] do
+    GV_ErrorIfNotValidLabel(labels[i]);
     GraphvizSetAttr(nodes[i], "label", labels[i]);
   od;
   return gv;

gap/gv.gd

@@ -60,8 +60,12 @@ DeclareOperation("GV_AddEdge",
 DeclareOperation("GV_GetIdx", [IsGraphvizObject]);
 DeclareOperation("GV_ConstructHistory", [IsGraphvizGraphDigraphOrContext]);
 
+DeclareOperation("GV_RemoveGraphAttrIfExists",
+[IsGraphvizGraphDigraphOrContext, IsString]);
+
 DeclareGlobalFunction("GV_IsValidColor");
 DeclareGlobalFunction("GV_ErrorIfNotNodeColoring");
+DeclareGlobalFunction("GV_ErrorIfNotValidLabel");
 
 # TODO move to dot? and make public?
 BindGlobal("GV_ObjectFamily",

gap/gv.gi

@@ -259,7 +259,7 @@ end);
 InstallMethod(Size, "for a graphviz map",
 [GV_IsMap], m -> Length(GV_MapNames(m)));
 
-# ??
+# Graph child counter functions
 
 InstallMethod(GV_IncCounter,
 "for a graphviz graph",
@@ -400,7 +400,7 @@ function(g, n)
   local graph;
   graph := GV_FindGraphWithNode(g, n);
   if graph = fail then
-    return graph;
+    return fail;
   fi;
   return graph[n];
 end);
@@ -464,6 +464,39 @@ function(x, edge)
   return x;
 end);
 
+InstallMethod(GV_RemoveGraphAttrIfExists,
+"for a graphviz graph context or digraph and a string",
+[IsGraphvizGraphDigraphOrContext, IsString],
+function(obj, attr)
+  local attrs, i, match;
+  attrs := GraphvizAttrs(obj);
+  attr  := String(attr);
+
+  # checks if they attribute names match the one being removed
+  match := function(key, str)
+    for i in [1 .. Length(key)] do
+      if i > Length(str) or key[i] <> str[i] then
+        return false;
+      fi;
+    od;
+
+    i := i + 1;
+    while i <= Length(str) do
+      if str[i] = '=' then
+        return true;
+      elif str[i] <> '\s' and str[i] <> '\t' then
+        return false;
+      fi;
+      i := i + 1;
+    od;
+
+    # attributes which are not key value or removal by value
+    return true;
+  end;
+
+  obj!.Attrs := Filtered(attrs, s -> not match(attr, s));
+end);
+
 ###############################################################################
 # Stringifying
 ###############################################################################
@@ -559,62 +592,42 @@ InstallMethod(GV_StringifyNodeEdgeAttrs,
 "for a GV_Map",
 [GV_IsMap],
 function(attrs)
-  local result, keys, key, val, n, i, tmp;
+  local result, keys, key, val, n, i, tmp, format;
 
   result := "";
   n      := Length(GV_MapNames(attrs));
   keys   := SSortedList(GV_MapNames(attrs));
 
+  # helper for formatting attribute kv pairs
+  format := function(format, key, val)
+    tmp := Chomp(val);
+    if "label" = key and StartsWith(tmp, "<<") and EndsWith(tmp, ">>") then
+      val := StringFormatted("{}", val);
+    else
+      if ' ' in key then
+        key := StringFormatted("\"{}\"", key);
+      fi;
+
+      if ' ' in val or '>' in val or '^' in val or '#' in val then
+        val := StringFormatted("\"{}\"", val);
+      fi;
+    fi;
+
+    return StringFormatted(format, key, val);
+  end;
+
   if n <> 0 then
     Append(result, " [");
     for i in [1 .. n - 1] do
         key := keys[i];
         val := attrs[key];
 
-        tmp := Chomp(val);
-        if "label" = key and StartsWith(tmp, "<<") and EndsWith(tmp, ">>") then
-          val := StringFormatted("{}", val);
-        else
-          # TODO it doesn't seem to be possible to enter the if-statement
-          # below, even with examples where the key contains spaces (probably
-          # the quotes are added somewhere else). Either uncomment or delete
-          # this code.
-          # if ' ' in key then
-          #   key := StringFormatted("\"{}\"", key);
-          # fi;
-          if ' ' in val or '>' in val or '^' in val or '#' in val then
-              # TODO avoid code duplication here, and below
-            val := StringFormatted("\"{}\"", val);
-          fi;
-        fi;
-
-        Append(result,
-               StringFormatted("{}={}, ",
-                               key,
-                               val));
+        Append(result, format("{}={}, ", key, val));
     od;
-
     # handle last element
     key := keys[n];
     val := attrs[key];
-
-    tmp := Chomp(val);
-    if "label" = key and StartsWith(tmp, "<<") and EndsWith(tmp, ">>") then
-      val := StringFormatted("{}", val);
-    else
-        if ' ' in key then
-          key := StringFormatted("\"{}\"", key);
-        fi;
-        if ' ' in val or '>' in val or '^' in val or '#' in val then
-          # TODO what are the allowed things in the value?
-          val := StringFormatted("\"{}\"", val);
-        fi;
-    fi;
-
-    Append(result,
-           StringFormatted("{}={}]",
-                           key,
-                           val));
+    Append(result, format("{}={}]", key, val));
   fi;
 
   return result;
@@ -669,11 +682,9 @@ function(graph, is_subgraph)
   elif IsGraphvizGraph(graph) then
     Append(result, "//dot\n");
     Append(result, GV_StringifyGraphHead(graph));
-    # TODO doesn't seem to be possible to reach the case below either, uncomment
-    # or delete
-    # else
-    #  Append(result, "//dot\n");
-    #  Append(result, GV_StringifyContextHead(graph));
+  else
+    ErrorFormatted("Unknown graph category, ",
+                   "expected a context, digraph or graph.");
   fi;
 
   Append(result, GV_StringifyGraphAttrs(graph));
@@ -737,3 +748,38 @@ function(gv, colors)
   fi;
   Perform(colors, ErrorIfNotValidColor);
 end);
+
+InstallGlobalFunction(GV_ErrorIfNotValidLabel,
+function(label)
+    local cond;
+
+    if Length(label) = 0 then
+        ErrorFormatted("invalid label \"{}\", valid DOT labels ",
+                       "cannot be empty strings", label);
+    fi;
+
+    # double quoted string
+    if StartsWith(label, "\"") and EndsWith(label, "\"")then
+        return;
+    fi;
+    # HTML string
+    if StartsWith(label, "<") and EndsWith(label, ">")then
+        return;
+    fi;
+
+    # numeral
+    if Int(label) <> fail then
+        return;
+    fi;
+
+    cond := not IsDigitChar(label[1]);
+    cond := cond and ForAll(label, c -> IsAlphaChar(c) or IsDigitChar(c)
+                            or c = '_' or ('\200' <= c and c <= '\377'));
+    if cond then
+        return;
+    fi;
+
+    ErrorFormatted("invalid label \"{}\", valid DOT labels ",
+                   "https://graphviz.org/doc/info/lang.html",
+                   label);
+end);

tst/dot.tst

@@ -29,7 +29,7 @@ gap> GraphvizAttrs(g);
 [ "color=red", "shape=circle" ]
 gap> GraphvizSetAttrs(g, rec(color := "blue", label := "test"));;
 gap> GraphvizAttrs(g);
-[ "color=red", "shape=circle", "label=test", "color=blue" ]
+[ "shape=circle", "label=test", "color=blue" ]
 
 # Test stringify
 gap> g := GraphvizGraph();;