xflr6 / graphviz

Simple Python interface for Graphviz
https://graphviz.readthedocs.io
MIT License
1.63k stars 211 forks source link

add NodeID class, used by quote and quote_edge #169

Closed bayersglassey-zesty closed 2 years ago

bayersglassey-zesty commented 2 years ago

Draft PR for now; the tests pass (including the new ones), but I didn't update the docs, and the docstring on NodeID could probably use some tightening up!

bayersglassey-zesty commented 2 years ago

@xflr6 If there is no interest in this approach, shall I close the PR?

xflr6 commented 2 years ago

Thanks for the proposal and sorry for the delay.

I still think that that allowing singleton tuples to triples would be a good way to introduce this new feature. Maybe we can discuss why you disprefer it? E.g., I don't think it's nice to require the user to use a custom class.

It's a bit more explicit, and allows you to pass "fully-qualified" node IDs (including port + compass) to node(), which wasn't previously possible. (And is also totally useless, because node statements ignore ports and compasses. But it's nice to be consistent.)

If compass and port in node statements are ignored, I am not sure why we would expose this possibility to the user.

bayersglassey-zesty commented 2 years ago

I still think that that allowing singleton tuples to triples would be a good way to introduce this new feature.

This PR is from some time ago, but I believe the reason I went with a custom class was because then I can control its __str__. That is, when I use the graphviz library, I usually hold node IDs in strings. However, unless you remember to escape the strings before storing them, they can end up getting "mangled" when quote is applied to them later on. So I thought that it might be nicer, as a user of the library, to store explicit NodeId objects instead:

graph = Digraph()

node_ids = {}

# Generate node ids, add nodes to graph
for thing in list_of_stuff:
    node_id = NodeId(...build unique id from thing...)
    node_ids[thing] = node_id
    graph.node(node_id)

# Add edges to graph
for thing in list_of_stuff:
    for linked_thing in thing.links:
        id1 = node_ids[thing]
        id2 = node_ids[linked_thing]
        graph.edge(id1, id2)

...that's the pattern I usually use, except usually I don't have a NodeId class, just raw strings. ...but I guess I could just use escape in place of NodeId. :thinking:

So okay, maybe we don't need NodeId or even tuples; it's enough to let user know about escape. But supporting tuple instead of raw string would be vaguely more user-friendly.