Repository navigation
Merge IsGraphvizGraph, IsGraphvizDigraph and IsGraphvizContext into a single type - #121
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #121 +/- ##
==========================================
- Coverage 94.90% 94.43% -0.47%
==========================================
Files 12 12
Lines 1255 1223 -32
==========================================
- Hits 1191 1155 -36
- Misses 64 68 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Is it still possible to disambiguate contexts from graphs and digraphs? This is important in the changes I'm making. |
Oui, |
|
Very good, I'll try to take a look today |
| if IsGraphvizDigraph(g) then | ||
| kind := "digraph"; | ||
| elif IsGraphvizContext(g) then | ||
| if g!.IsContext then |
There was a problem hiding this comment.
Isn't this just poor man's properties? I.e. shouldn't we just define IsGraphvizContext as a property of objects in the category IsGraphvizGraph, and likewise IsGraphvizDigraph? This is equivalent to what you're doing here in many ways, and is more "official". O/w I approve these changes.
james-d-mitchell
left a comment
There was a problem hiding this comment.
Please change !.IsContext etc to properties of IsGraphvizGraph, then I'm happy to merge this!
dc95572 to
0bff575
Compare
|
Turned out it wasn't possible to use properties, so I re-added |
d282752 to
9d355e5
Compare
|
In discussion with @reiniscirpons we decided to revert |
9d355e5 to
41ed3e6
Compare
|
The rebase was non-trivial, it looks like I haven't completed it properly, please don't merge until I can come back to it. |
41ed3e6 to
fcdf8ec
Compare
This PR unifies
IsGraphvizGraph,IsGraphvizDigraphandIsGraphvizContextinto a single typeIsGraphvizGraph.We could recover these types by re-adding them as properties, but since all of these types share the same interface, there is no need to have different constructors and filters for them.
Fixes #115 .