-
Notifications
You must be signed in to change notification settings - Fork 767
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
Expose GraphvizFormatting and test in python #1059
Expose GraphvizFormatting and test in python #1059
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome! I'll trigger CI so please wait on the results before pushing, so CI only has to run once more if any problems pop up there.
Thanks for your positive feedback! One additional thing I just noticed: in the readme of the python directory it is written it should take CMAKE_INSTALL_PREFIX into account. But according to my observation, it does not. I think we have to add Edit: Obviously, I can push that if you say we want that and the CI is done ;) |
@varunagrawal comment? |
I actually like the option to install it somewhere else. By default, it wants to install to |
I need to check but that part of the readme may indeed be out of date. |
Changed copyright section and included --prefix option for setup.py. I think the second point only is beneficial and has no drawbacks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, but the prefix thing has to be a different PR. We can't risk unintended consequences. Please remove from this PR, but we'd love it if you could submit as a separate PR and explain the behavior. I don;t understand well enough what it does.
Thanks !!!!! |
This exposes the
GraphvizFormatting
class with direct member access as discussed in #1053. This allows to do some configuration on thesaveGraph
call that was not possible via the bindings before.It also adds a few basic tests for it.
Lastly, I had the problem that the python-test target was only testing the files that were copied to the build directory before but not any new tests. Adding the dependency fixed that by always copying the tests. I don't know if that is wanted behavior.