Implement RST directive for graphviz #35
Conversation
|
Thanks for this PR, as well as for #33. The changes are quite substantial and will take some time to review. Please bear with me. |
|
Sure, thanks for reviewing! No rush. |
|
P.S. to clarify, I was just sending this draft as an FYI about what I had in mind to build on top of #34. (Though we could instead keep the whole discussion in this context of this PR, whichever workflow you prefer :) ) As for the test refactoring bit, in hindsight the change to setUp parameters isn't as necessary as I'd originally thought. In fact, I think I could drop that commit and keep the rest of the stack more or less the same. The downside to dropping that commit would be that it'd complicate adding additional tests I'd like (not yet in this draft PR) to, e.g., make sure my translation of the boolean |
I overlooked the fact that this PR was still marked as Draft. Thank you for providing the context. I propose we resume the discussion once you have marked the PR as ready for review. I am now currently reviewing PR #34.
I prefer to keep the discussions separate.
Do not worry about the size of the changes as long as you keep them atomized, which you are already doing. |
This factors out the logic used to create the base64-compressed img element, which can be reused in the RST path. Unfortunately, I didn't see a good way to similarly factor out the non-compressed code. The problem is that the Markdown path relies on working with ElementTree, and there setting the raw SVG output as the container's `text` apparently works; but in the later-added RST path, setting the SVG as `text` causes it to be escaped when I then pass it to ET.tostring. (I think this could be solved by using lxml instead, but that would require a bigger change to the existing Markdown path.)
This adds a `graphviz` RST directive, supporting the same settings and options as the Markdown version. Unlike in the Markdown version, the name of the directive is not currently configurable.
Adds a test_rst() method to TestGraphviz so that the existing test cases also run for reStructuredText input.
Adds tests cases to ensure the "truthy" value parsing in the RST directive handles "yes" and "no" values like the Markdown extension.
Fixes a doc string typo
rlaboiss
left a comment
rlaboiss
left a comment
There was a problem hiding this comment.
I reviewed your changes. Everything seems fine for me. Thanks for this great improvement in the graphviz plugin! Thanks also for adding the two new tests for checking the compress option.
I am going to merge this PR. I will do some extra tweaks, improve the wording the the README.md file and make a new release of the plugin.
|
Sounds great, thanks again! |
|
It is done. I did a major bump in the version number (2.0.0). The plugin should now be available at PyPI. It would be great if you could test the installed version of the plugin on your personal blog. |
|
Awesome, thanks for the fixes and for publishing the new release. I tested this by changing my blog to depend on |
2 participants
Stacked on top of #34
For #32, this moves some additional shared logic into run_graphviz.py, then implements the reStructuredText directive, enables tests, and documents how to use it in the README.
One limitation here is that as opposed to the Markdown version, the directive name is not currently configurable. I could add that if you'd like, but maybe it'd have to be a different setting name, since the Markdown block setting includes the leading dots?