Resolve Issue 75 by james-d-mitchell · Pull Request #77 · gap-packages/GraphvizForGAP

james-d-mitchell · 2026-09-29T09:21:30Z

This should resolve #75, I also added normalization of the whitespace in GraphvizSetAttr, which meant that some of the tests needed to be rewritten.

codecov · 2026-09-29T09:23:59Z

Codecov Report

❌ Patch coverage is 86.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.24%. Comparing base (f121348) to head (29e83fe).
⚠️ Report is 20 commits behind head on main.

Files with missing lines	Patch %	Lines
gap/dot.gi	85.71%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #77      +/-   ##
==========================================
- Coverage   90.30%   90.24%   -0.06%     
==========================================
  Files          12       12              
  Lines        1289     1302      +13     
==========================================
+ Hits         1164     1175      +11     
- Misses        125      127       +2

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ruthhoffmann

This might sound silly, but
do you really need to "overload" the function for each type of Graphviz concept? Seems a bit inefficient, but if that's the way then I'm OK with this PR.

james-d-mitchell · 2026-09-29T09:51:21Z

This might sound silly, but do you really need to "overload" the function for each type of Graphviz concept? Seems a bit inefficient, but if that's the way then I'm OK with this PR.

Good point, perhaps this isn't the correct solution, let me look at the code some more (I'm going to a lecture from 11 to 12, but back after that).

james-d-mitchell · 2026-09-29T11:58:09Z

@ruthhoffmann looking into this a little further, it seems that the return value of GraphvizAttrs is a list for IsGraphvizGraphDigraphOrContext but a record for IsGraphvizNodeOrEdge so we need different code to handle these. It'd probably be better if GraphvizAttrs always returned a record, and then this code was deduplicated, but are we happy with this for now, and I will attempt to refactor so that GraphvizAttrs is always a record?

ruthhoffmann · 2026-09-30T08:27:25Z

The plan from yesterday sounds good to me! (github was dodgy and this comment must not have sent yesterday apologies!)

dvberkel

I would love to see a small update to the error message

dvberkel · 2026-09-30T08:43:38Z

+  attr := SplitString(str, "=");
+  if Length(attr) <> 2 then
+    ErrorFormatted("expected a string of the form \"attribute_name=value\", ",
+                   "but the input string {} does not contain \"=\"", str);


If the input string contains more than 1 =, e.g. "i=like=bananas", it will also fail, with the message

expected a string of the form "attribute_name=value" but the input string i=like=bananas does not contain "="

This is obviously an incorrect statement.

An alternative could be

expected a string of the form "attribute_name=value" but the input string i=like=bananas does not adhere

We agreed with @dvberkel that its probably best to skip argument checking in the 2 argument form at the moment for simplicity. So we leave it up to the user to supply a reasonable 2nd argument and simply add it to the attrs array as is.

I'm not sure that that is consistent with the other methods though, I'm off to teach until 2, so can't do anything right away. I think "parsing" the string is consistent with the other 2 arg SetAttr, which is a little further down the same file (and probably does a better job than the version in this PR).

james-d-mitchell · 2026-10-01T13:27:24Z

Thinking about this a bit more, I think we are using the term "attribute" too loosely in the package so far. I think when we are doing things like

GraphvizSetAttr(g, "node [shape=box]");

we are really just telling GraphvizForGAP to insert the string "node [shape=box]" into the output verbatim. A more systematic solution would be to separate these completely into

GraphvizSetAttr(g, "color", "red");
GraphvizSetAttrs(g, rec(label := "My graph", rankdir := "LR"));

GraphvizAddRaw(g, "node [shape=box]");

this would also subsume #38 since I'd be able to do things like:

GraphvizAddRaw(g, "// this is amazing");

If we make this change, then every object can have an Attrs record (so not a list as is currently the case for graphs, digraphs, and contexts), and graphs, digraphs, and contexts can have a Raw list also.

One downside is:

We could do GraphvizAddRaw(g, "color=blue"); and GraphvizRemoveAttr(g, "color") will not remove the color=blue (since one is in Raw, and the other in Attrs).

I'd propose to then remove the 2-arg version of GraphvizSetAttr and only permit GraphvizSetAttr(g, key, val).

We could then additionally implement:

GraphvizSetNodeDefaults(g, rec(shape := "box", color := "blue"));
GraphvizSetEdgeDefaults(g, rec(style := "dashed"));

as helpers.

What do you think @reiniscirpons, @ruthhoffmann, and @dvberkel ?

fingolfin · 2026-10-01T13:32:48Z

To me GraphvizSetNodeDefaults(g, rec(shape := "box", color := "blue")); sounds quite good

james-d-mitchell · 2026-10-01T13:51:29Z

Thanks @fingolfin any opinion on the other things proposed?

reiniscirpons · 2026-10-01T14:04:34Z

@james-d-mitchell

This is a good suggestion. Vis a vis nomenclature @ruthhoffmann thinks that something like GraphvizSetSubgraphNodeAttrs would be better, since the term Default seems to imply a global default, however the node [bla] attribute is scoped to the current context or subgraph.

Also, in the name of simplicity we could also have the regular GraphvizSetAttrs provide this functionality by special casing the attributes named graph, node, edge, so we could do something like

GraphvizSetAttrs(g, rec(node := rec(shape := "box", color := "blue")));

Which would be equivalent to

GraphvizSetSubgraphNodeAttrs(g, rec(shape := "box", color := "blue"));

Looking at the graphviz lang spec (https://graphviz.org/doc/info/lang.html), there are actually only 3 kinds of "scoped" attributes that can be set to apply to the subgraph as well as its nodes or edges:

attr_stmt: (graph | node | edge) attr_list

And these are reserved keywords so we cannot have an attribute named like this, and it should be safe.
I also dont think that Raw is neccesary if we do it like this.

I agree with making every Attrs variable a record, this cuts down on the special casing.

james-d-mitchell · 2026-10-01T15:13:33Z

How about GraphvizSetNodeAttrs? Since GraphvizSetSubgraphNodeAttrs also doesn't quite capture what this does. I.e. you can call GraphvizSetNodeAttrs multiple times on the same graphviz graph (say), and then those attributes only apply to the nodes defined after this function is called. This doesn't only apply to subgraphs (which mean something specific in graphviz land).

I think I'll start by making GraphvizAttrs always return a record, and then try to figure out what to do next.

reiniscirpons · 2026-10-01T15:27:43Z

GraphvizSetNodeAttrs Seems to imply we will be setting the attrs of a particular node rather than for all nodes belonging to the subgraph.

GraphvizSetNodeAttrs multiple times on the same graphviz graph (say), and then those attributes only apply to the nodes defined after this function is called.

Do we need to preserve this behavior? We provide mechanisms now via contexts etc where if we wanted to have some nodes be styled one way, we simply make a context for them where they get colored and move on. I'm in general not sure we should allow a user to create an arbitrary graphviz file, rather we should restrict it to a sane subset of the language I think.

What do you think @ruthhoffmann @dvberkel?

Specifically the question is whether we want something like

g := GraphvizGraph();
GraphvizAddNode(g, 1);
GraphvizAddNode(g, 2);
GraphvizSetAttr(g, "node [style=filled]");
GraphvizAddNode(g, 3);
GraphvizAddNode(g, 4);

to produce

graph g {
  1
  2
  nodes [style=filled]
  3
  4
}

or

graph g {
  nodes [style=filled]
  1
  2
  3
  4
}

The first will have nodes 1 and 2 plain and nodes 3 4 filled, but the second will have all four nodes filled.

I think I'll start by making GraphvizAttrs always return a record, and then try to figure out what to do next.

This seems good

james-d-mitchell · 2026-10-01T15:38:10Z

I'm trying to untangle these two things, but it's seemingly not possible. So, I think I'll implement GraphvizSetDefaultXYZ and we can quibble about the names after.

ruthhoffmann · 2026-10-01T15:39:17Z

~~I can see why both options that @reiniscirpons has presented could be expected.~~ (I take this back!)
He just checked and the latter is what is implemented.

The former should only be ever done with subgraphs or contexts.

james-d-mitchell · 2026-10-01T15:41:33Z

Agreed, I'm implementing

james-d-mitchell · 2026-10-01T16:19:05Z

This is a fairly big change, and I'm getting a bit tired, so I'm going to stop after I type this for today. It seems to me that we should actually have 3 concepts here:

node attributes which can be applied to a graph, digraph, context, or node
edge attributes which can be applied to a graph, digraph, context, or edge
graph attributes which can be applied to a graph or digraph (but not a context, node or edge)

As such it might make sense to remove GraphvizSetAttr, GraphvizSetAttrs, GraphvizGetAttr, and GraphvizAttrs, and instead have

GraphvizSetNodeAttr, GraphvizSetNodeAttrs, GraphvizNodeAttr, and GraphvizNodeAttrs (the latter 2 being the getters following the usual GAP convention) for graph, digraph, context, or node
GraphvizSetEdgeAttr, GraphvizSetEdgeAttrs, GraphvizEdgeAttr, and GraphvizEdgeAttrs for
graph, digraph, context, or edge
GraphvizSetGraphAttr, GraphvizSetGraphAttrs, GraphvizGraphAttr, and GraphvizGraphAttrs for graph and digraphs.

What do you guys think? Since this is a pretty major change, I wanted to check first. This will also interact really badly if you resolve #115 concurrently, so it might be worthwhile holding off on that.

reiniscirpons · 2026-10-02T07:56:57Z

It seems to me that we should actually have 3 concepts here:

This is sort-of true but the grouping of nodes and the collections of nodes, and similarly edges and sets of edges is too much. According to the language spec (https://graphviz.org/doc/info/lang.html) we have two concepts:

Attributes can apply to:
1.1. A single node,
1.2. A single edge,
1.3. The collection of all nodes of a subgraph,
1.4. All edges of a subgraph or
1.5. A graph and all its subgraphs,

These are always denoted by an attribute list of the form [attr1=val1, attr2=val2, ..., attrn=valn], but what changes is what needs to precede the list in the output:

For each object of the previous kind there is a single way of applying attributes it:
2.1. For a node, the attrs must follow the node n1 [attr1=val1, ...],
2.2. For an edge, the attrs must follow the edge n1 -- n2 [attr1=val1, ...]
2.3. For the collection of all nodes of a subgraph we write node followed by the attributes node [attr1=val1, ...]
2.4. For the collection of all edges we do edge [attr1=val1, ...]
2.5. For the graph and all its subgraphs we do graph [attr1=val1, ...]

In particular, I think we should keep GraphvizSetAttr, GraphvizSetAttrs, GraphvizGetAttr and GraphvizAttrs as generic functions for setting attributes for either nodes, edges or graphs (if/when we add html stuff we an also set html attrs with it too).

Then the functions GraphvizSetNodeAttr, GraphvizSetEdgeAttr, GraphvizSetGraphAttr etc can just be convenience wrappers around GraphvizSetAttr.

james-d-mitchell · 2026-10-02T08:17:52Z

Thanks for the comments @reiniscirpons, with your proposal, if I have a graph or digraph, how would I set an attribute for all nodes? I.e. how would I specify this to GraphvizSetAttr?

reiniscirpons · 2026-10-02T08:27:06Z

@james-d-mitchell I think we should do a meeting and discuss this, I sent you a message on Signal.

But to answer your question we special case the attributes node, edge and graph for GraphvizGraph so that
GraphvizSetAttrs(G, rec(node := rec(color := "red"))) sets the node [color="red"] attribute.

james-d-mitchell · 2026-10-02T09:21:12Z

We have a way forward, I'll implement when I'm back from class.

reiniscirpons · 2026-10-02T10:47:44Z

@james-d-mitchell looks like I wont finish my PR for #115 in time, so go ahead with this PR, I will incorporate changes you make into my PR after.

james-d-mitchell · 2026-10-03T13:57:48Z

Superseded by #122

james-d-mitchell requested a review from ruthhoffmann September 29, 2026 09:21

ruthhoffmann reviewed Sep 29, 2026

View reviewed changes

james-d-mitchell added 2 commits September 30, 2026 09:07

Resolve Issue 75

6c1414b

Increase line length to 100

29e83fe

james-d-mitchell force-pushed the fix-issue-75 branch from b708b5c to 29e83fe Compare September 30, 2026 08:08

dvberkel requested changes Sep 30, 2026

View reviewed changes

james-d-mitchell mentioned this pull request Oct 1, 2026

Remove getters and setters using square brackets #116

Merged

james-d-mitchell mentioned this pull request Oct 3, 2026

gv: attrs are always a record #122

Merged

james-d-mitchell closed this Oct 3, 2026

james-d-mitchell deleted the fix-issue-75 branch October 3, 2026 13:57

Conversation

james-d-mitchell commented Sep 29, 2026

Uh oh!

codecov Bot commented Sep 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ruthhoffmann left a comment

Choose a reason for hiding this comment

Uh oh!

james-d-mitchell commented Sep 29, 2026

Uh oh!

james-d-mitchell commented Sep 29, 2026

Uh oh!

ruthhoffmann commented Sep 30, 2026

Uh oh!

dvberkel left a comment

Choose a reason for hiding this comment

Uh oh!

dvberkel Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

reiniscirpons Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

james-d-mitchell Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

james-d-mitchell commented Oct 1, 2026

Uh oh!

fingolfin commented Oct 1, 2026

Uh oh!

james-d-mitchell commented Oct 1, 2026

Uh oh!

reiniscirpons commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

james-d-mitchell commented Oct 1, 2026

Uh oh!

reiniscirpons commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

james-d-mitchell commented Oct 1, 2026

Uh oh!

ruthhoffmann commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

james-d-mitchell commented Oct 1, 2026

Uh oh!

james-d-mitchell commented Oct 1, 2026

Uh oh!

reiniscirpons commented Oct 2, 2026

Uh oh!

james-d-mitchell commented Oct 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

reiniscirpons commented Oct 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

james-d-mitchell commented Oct 2, 2026

Uh oh!

reiniscirpons commented Oct 2, 2026

Uh oh!

james-d-mitchell commented Oct 3, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov Bot commented Sep 29, 2026 •

edited

Loading

reiniscirpons Sep 30, 2026 •

edited

Loading

reiniscirpons commented Oct 1, 2026 •

edited

Loading

reiniscirpons commented Oct 1, 2026 •

edited

Loading

ruthhoffmann commented Oct 1, 2026 •

edited

Loading

james-d-mitchell commented Oct 2, 2026 •

edited

Loading

reiniscirpons commented Oct 2, 2026 •

edited

Loading