-
Notifications
You must be signed in to change notification settings - Fork 64
Description
The dot parser src/dot.ml contains small glitches, and I am happy to fix them if it sounds good to you.
My starting point was to add support for Equal of id * id defined in dot_ast.mli but not handled in the dot parser. e.g. label="diagram_label" bgcolor=red bgcolor=lightgrey label="cluster_1" in this examples.
digraph D {
label="diagram_label";
bgcolor=red;
node [color=green];
edge [penwidth=3];
subgraph cluster_1 {
bgcolor=lightgrey;
label="cluster_1";
}
}
These graph attributes are ignored by the current parser. It makes it difficult to provide in the val graph_attributes: t -> DotAttributes.graph list that is used in module DotOutput = Graphviz.Dot(Display).
This Equal binds attributes to the wrapping top-graph / subgraph. Then I realized there is no data structures to hold these attributes, which takes a subgraph as key.
In #129, I added such a structure graph_hash and followed the approach of the existing code, using Map internally and converting to a list before returning. (Is it necessary?)
I wonder how much change is welcomed to the existing code. Then I can change my PR, or just modify on my own branches.
I understand the subtlety comes from graphviz contains nodes, edges, and (sub)graphs while graph in OCamlGraph contains only nodes and edges.
Here are my other questions:
Q1. val get_subgraph : V.t -> DotAttributes.subgraph option (in Graphviz.Dot.X) is not well documented. I can make one from the result of my parse_all but it still looks tedious to me.
Q2. subgraph doesn't have to be named as cluster_<id> but it's assumed by print_nested_subgraphs (line 529). (but to accommodate this, I have to believe the subgraph must follow this convention in my #129
Q3. For node and edge attributes in dot file, a subgraph inherits attributes from its parent graph, however, changes inside a subgraph won't affect the nodes after the subgraph. (See my example dot file. I think the current implementation in the dot parser is incorrect because it uses one mutable node_attr. (An immutable map is easier to handle this case in the scenario).