Issue #33: Fix directed edges #34
Conversation
|
Thanks for this PR, You are introducing quite a lot of other things in this PR than just a fix for this issue you found. Please remove those. And make this a clean PR to fix the issue. Any additional changes can be discussed but should not be in this pr. It is perfectly ok if you are using tools to improve your own code base, but I don't want them in this repo right now. If we want to introduce Grumphp for example I want to use it organisation wide and define how to introduce it. Not just in a single repo, without any additional that is organisation wide used. |
|
Allrighty, removed all the unrelated stuff. |
|
Waiting for your feedback @jaapio . |
There was a problem hiding this comment.
First of all, you are changing quite a lot, introducing interfaces that have never been there, adding references to the Graph where I don't expect any reference.
Please make clear first in the issue you are trying to fix why a change is needed. I'm not familiar with the format that this library implements. But since it works fine for phpDocumentor I'm wondering why the current implementation is wrong.
After that, we could have a look at how this could be implemented in the library. I think there is an easier way to achieve this.
src/phpDocumentor/GraphViz/Edge.php
Outdated
| @@ -123,8 +126,16 @@ public function __toString() : string | |||
| $from_name = addslashes($this->getFrom()->getName()); | |||
| $to_name = addslashes($this->getTo()->getName()); | |||
|
|
|||
| $direction = '--'; | |||
|
|
|||
| if (null !== $graph = $this->getGraphRoot()) { | |||
There was a problem hiding this comment.
I don't get why this is needed... you are referencing the graph in the Edge class which makes it harder to cache the objects because of the circular reference.
Only thing you need is a direction attribute on the edge. I'm not very familiar with the format this library is implementing. But I don't think this is the correct way of doing this. If feels like a very high impact on the library and usability of it. While it is unclear to me why this is needed.
There was a problem hiding this comment.
I know and I agree.
What would be the best way, according to you, to implement this ?
A third parameter in the constructor?
There was a problem hiding this comment.
I think that would be a better approach since you are basically setting the direction of the arrow.
maybe even add a few extra named constructors to help the user choosing the right value for $direction?
There was a problem hiding this comment.
Yeah we could do that as well :-)
There was a problem hiding this comment.
the problem by doing that is... let's take the following scenario:
- You create some undirected edges
- You create a digraph
- You add the edges to the graph
- You export the file and try to pass it through
dot... and it will fail.
There was a problem hiding this comment.
sounds to me that we should throw an exception in that case. A graph with the type Digraph can only contain DirectedEdges We could model that into this library, with more specialized types.
There was a problem hiding this comment.
Ok, why not. I will see if I can rework my PR.
|
Is there any chance this is moving forward - and changes into master? |
This PR:
.editorconfigconfiguration file..gitattributesfile.