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

Allow setting of graph name via graphviz.name attribute #20

Closed
wants to merge 2 commits into from

Conversation

rhelms
Copy link
Contributor

@rhelms rhelms commented Apr 6, 2017

#3

@clue
Copy link
Member

clue commented Apr 6, 2017

Thanks for filing this PR! 👍

Does it make sense to add this to the documentation so people learn about this feature? Also, in the linked ticket you stated that you found some documentation why name may be preferred over id, can you elaborate on this? Again, thanks!

@rhelms
Copy link
Contributor Author

rhelms commented Apr 6, 2017 via email

@rhelms
Copy link
Contributor Author

rhelms commented Apr 7, 2017

With regards to the usage of name vs id, the man pages use name and the abstract grammar uses ID.

If the Fhaculty\Graph\Graph accepted an id in the constructor or had it changeable as a property, then I would have used the constructor or methods to set the id and be consistent with Vertex.

But since those are not there, and the GraphViz class is something else, going with what is in the manual pages for the installed product seemed the next best thing, for consistency.

However, if you'd really like to see it as id, I'd be happy to make that change. I'm just glad there's an easy to use API out there, given that PHP 7 bindings have been removed from GraphViz for the moment.

@clue
Copy link
Member

clue commented Oct 9, 2018

@rhelms Again thank you for filing this PR and for your patience!

I agree that this feature makes perfect sense and have just merged a slightly modified version with #28 (with proper attribution of course) :shipit: 🎉

Thanks for your investigation! Interestingly, https://www.graphviz.org/doc/info/lang.html and also https://graphviz.readthedocs.io/en/stable/manual.html#basic-usage suggest that the root graph name is actually optional. Accordingly, I've changed this to omit the default "G" root graph name by default and otherwise added documentation for the new graphviz.name attribute as introduced in this PR.

This means that this now works as expected out of the box and there should be no need to change this for most common use cases. Again, thank you! 👍

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

Successfully merging this pull request may close these issues.

2 participants