Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve the (graphviz) dot parser #130

Open
arbipher opened this issue Nov 26, 2022 · 0 comments
Open

Improve the (graphviz) dot parser #130

arbipher opened this issue Nov 26, 2022 · 0 comments

Comments

@arbipher
Copy link

arbipher commented Nov 26, 2022

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant