parse subgraphs recursively #72
Conversation
Before, only one subgraphlevel was added correctly, as pydot.get_subgraph_list only returns the subgraphs at the current level. I put the node and edge parsing into a separate function and made them recursive.
This was only relevant for developing
|
That sounds like a good improvement. Can you please provide a set of steps to exercise the new functionality. |
|
It's easiest to check using a unit test. I'll upload one shortly, but first #77 could be merged, maybe, as I added the unittest inside that file. |
|
I added an example test case with a graph with two nested subgraphs. If this test is run on the kinetic-devel branch, it will fail, as the edge inside the most inner subgraph can't be created, as the nodes never got parsed. |
| @@ -295,5 +304,26 @@ def dotcode_to_qt_items(self, dotcode, highlight_level, same_label_siblings=Fals | |||
| self.addEdgeItem(edge, nodes, edges, | |||
| highlight_level=highlight_level, | |||
| same_label_siblings=same_label_siblings) | |||
| return edges | |||
|
|
|||
| def dotcode_to_qt_items(self, dotcode, highlight_level, same_label_siblings=False): | |||
There was a problem hiding this comment.
Please do not move code around since it makes the diff much more complicated to review.
| @@ -237,34 +241,27 @@ def addEdgeItem(self, edge, nodes, edges, highlight_level, same_label_siblings=F | |||
| sibling.add_sibling_edge(edge_item) | |||
|
|
|||
| if label not in edges: | |||
There was a problem hiding this comment.
The check seems to not match the updated assignment in the next lines.
| edge_name = source_node.strip('"\n"') + '_TO_' + destination_node.strip('"\n"') | ||
| if label is not None: | ||
| edge_name = edge_name + '_' + label | ||
|
|
There was a problem hiding this comment.
Please move these lines above line 243 since edge_name is not used until then.
|
Sorry, it took me a while to get some time for this... @dirk-thomas: I tried to follow your suggestions for the PR |
| graph.subgraphs_iter = graph.get_subgraph_list | ||
|
|
||
| num_subgraphs = len(graph.get_subgraph_list()) |
There was a problem hiding this comment.
This variable is not used anywhere?
| nodes = {} | ||
| for subgraph in graph.subgraphs_iter(): | ||
| subgraph_nodeitem = self.getNodeItemForSubgraph(subgraph, highlight_level) | ||
| num_subgraphs = len(subgraph.get_subgraph_list()) |
There was a problem hiding this comment.
This variable is not used anywhere?
| nodes = {} | ||
| for subgraph in graph.subgraphs_iter(): | ||
| subgraph_nodeitem = self.getNodeItemForSubgraph(subgraph, highlight_level) | ||
| num_subgraphs = len(subgraph.get_subgraph_list()) | ||
|
|
||
| nodes = dict(nodes, **self.parse_nodes(subgraph, highlight_level)) |
There was a problem hiding this comment.
I think the following would be more readable and make the intention of the line clearer:
nodes.update(...)
Same below for edges.
There was a problem hiding this comment.
@dirk-thomas thanks for the really thorough review. I included the mentioned changes.
|
I have independently made these changes in #87. |
|
I am sorry for the long delay. I have just tried the current state and it looks good to me. Thank you for contributing this patch. |
Before, only one subgraphlevel was added correctly, as pydot.get_subgraph_list
only returns the subgraphs at the current level.
I put the node and edge parsing into a separate function and made them recursive.