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

Add _repr_mimebundle_ for IPython.display #150

Merged
merged 3 commits into from
Nov 19, 2021
Merged

Conversation

boeddeker
Copy link
Contributor

@boeddeker boeddeker commented Nov 15, 2021

Address #138

@xflr6 Thank you for the modifications of the graphviz code base.
I followed your suggestion and implemented the _repr_mimebundle_ .
For SVG, PNG and JPEG it works, but PDF and JSON didn't work.
In the IPython documentation is not much info, so I was not able to figure out, why they don't work.
(For PDF, they may expect a file)

I didn't change the name for JupyterSvgIntegration, because I wanted to wait for feedback.

Copy link
Owner

@xflr6 xflr6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR.

Implementation looks good in general. :)

  • I think _repr_mimebundle_() does the right thing. Proposed some simplifications
  • We will need to move the code in place a bit and make it more consistent with other parts, see my suggestions. Hope this helps.
  • Can you add type annotations?

Then there is the whole side of testing and documentation: It would be great if you can try to take a look at the tests and the documentation (see also https://graphviz.readthedocs.io/en/latest/development.html) and at least make sure no tests are broken and there is at least some code coverage of the added (non-module-level) code.But if you prefer me to take over, that is fine as well. Feel free to propose documentation and changelog entry. Maybe it would make sense to add a docstring and include the method in the API docs (treat it as pseudo-private), WDYT?

Please check flake8 in tests result (when they pass again).

graphviz/jupyter_integration.py Outdated Show resolved Hide resolved
graphviz/jupyter_integration.py Outdated Show resolved Hide resolved
graphviz/jupyter_integration.py Outdated Show resolved Hide resolved
graphviz/jupyter_integration.py Outdated Show resolved Hide resolved
graphviz/jupyter_integration.py Outdated Show resolved Hide resolved
graphviz/jupyter_integration.py Outdated Show resolved Hide resolved
graphviz/jupyter_integration.py Outdated Show resolved Hide resolved
graphviz/jupyter_integration.py Outdated Show resolved Hide resolved
graphviz/jupyter_integration.py Outdated Show resolved Hide resolved
@boeddeker
Copy link
Contributor Author

Then there is the whole side of testing and documentation: It would be great if you can try to take a look at the tests and the documentation (see also https://graphviz.readthedocs.io/en/latest/development.html) and at least make sure no tests are broken and there is at least some code coverage of the added (non-module-level) code.But if you prefer me to take over, that is fine as well. Feel free to propose documentation and changelog entry. Maybe it would make sense to add a docstring and include the method in the API docs (treat it as pseudo-private), WDYT?

I will check the development side and add some tests. I skipped them, because I was not sure, if my suggestion is ok.

I have a question regarding the parameters folder. Should I create a new file in parameters and write code similar to the other files there? Do you have a suggestion for the name? jupyter_representation.py and class JupyterRepresentation: ... is lengthy and juypter alone not informative.

Btw. when you think it takes less time for you, when you take over instead of suggesting changes, feel free to do so.
(I enabled "Allow edits by maintainers", so you can apply changes directly to this PR.)

Can you add type annotations?

Sure.

Please check flake8 in tests result (when they pass again).

Sure, I try to get all test working, including the style checks.

@xflr6
Copy link
Owner

xflr6 commented Nov 17, 2021

I skipped them, because I was not sure, if my suggestion is ok.
when you think it takes less time for you, when you take over instead of suggesting changes, feel free to do so.

All good. I am fine with making suggestions for now (can take over when one/both of us think it's time).

Should I create a new file in parameters and write code similar to the other files there?

I would say no: this submodule is stricly for parameters for rendering:

"""Hold and verify parameters for running Graphviz ``dot``."""

@boeddeker
Copy link
Contributor Author

I tried now to be as close as possible to the code you mentioned.

In tests/test_parameters.py I added verify_jupyter_representation.
From the naming, it does not fit there, but I didn't want to copy that code.

@xflr6
Copy link
Owner

xflr6 commented Nov 19, 2021

Thanks. Can you squash the PR into one commit?

If you are fine with me moving things, adapting naming and possibly more things, I can take over (if you have enabled it, I can add another commit to the PR branch so we can keep things separate after merging). SG?

pytype:

FAILED: /home/runner/work/graphviz/graphviz/.pytype/pyi/graphviz/jupyter_integration.pyi 
/opt/hostedtoolcache/Python/3.9.8/x64/bin/python -m pytype.single --imports_info /home/runner/work/graphviz/graphviz/.pytype/imports/graphviz.jupyter_integration.imports --module-name graphviz.jupyter_integration -V 3.9 -o /home/runner/work/graphviz/graphviz/.pytype/pyi/graphviz/jupyter_integration.pyi --analyze-annotated --gen-stub-imports --nofail --quick /home/runner/work/graphviz/graphviz/graphviz/jupyter_integration.py
File "/home/runner/work/graphviz/graphviz/graphviz/jupyter_integration.py", line 60, in JupyterSvgIntegration: Type annotation for include does not match type of assignment [annotation-type-mismatch]
  Annotation: list (Did you mean 'typing.Optional[list]'?)
  Assignment: None
File "/home/runner/work/graphviz/graphviz/graphviz/jupyter_integration.py", line 60, in JupyterSvgIntegration: Type annotation for exclude does not match type of assignment [annotation-type-mismatch]
  Annotation: list (Did you mean 'typing.Optional[list]'?)
  Assignment: None

@boeddeker
Copy link
Contributor Author

Thanks. Can you squash the PR into one commit?

Yes, I squashed now all changes.

If you are fine with me moving things, adapting naming and possibly more things, I can take over (if you have enabled it, I can add another commit to the PR branch so we can keep things separate after merging). SG?

Yes, you can take over. I fixed the typing you mentioned and renamed JupyterSvgIntegration to JupyterIntegration in my last commit.

@codecov-commenter
Copy link

codecov-commenter commented Nov 19, 2021

Codecov Report

Merging #150 (acf79bc) into master (7820519) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #150   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           32        32           
  Lines          860       891   +31     
=========================================
+ Hits           860       891   +31     
Impacted Files Coverage Δ
graphviz/graphs.py 100.00% <ø> (ø)
graphviz/sources.py 100.00% <ø> (ø)
graphviz/__init__.py 100.00% <100.00%> (ø)
graphviz/jupyter_integration.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7820519...acf79bc. Read the comment docs.

@xflr6 xflr6 merged commit acf79bc into xflr6:master Nov 19, 2021
inmantaci pushed a commit to inmanta/inmanta-core that referenced this pull request Nov 29, 2021
Bumps [graphviz](https://github.com/xflr6/graphviz) from 0.18.2 to 0.19.
<details>
<summary>Changelog</summary>
<p><em>Sourced from <a href="https://github.com/xflr6/graphviz/blob/master/CHANGES.rst">graphviz's changelog</a>.</em></p>
<blockquote>
<h2>Version 0.19</h2>
<p>Add <code>PendingDeprecationWarning</code> to calls using positional arguments
that will be <strong>deprecated in a later version</strong>.
The future API will allow from one to three positional arguments
depending on the method or function.
Keyword-only arguments where not around when this library was created.
This signals dependents and in general users to start updating
or pinning to the wanted version (or range).
Crucially, this helps new users with a safer API
that allows to avoid some common mistakes.
Warnings reported in tests.</p>
<p>Add keyword-only <code>outfile</code> argument to <code>.render()</code>
and stand-alone <code>graphviz.render()</code>.
Allows to override the rendered output file name:
<code>.render(filename='spam.gv', outfile='spam.pdf')</code>
Allows to derive the <code>format</code> and the <code>filename</code>
from the rendered <code>outfile</code> name:
<code>.render(outfile='spam.svg')</code>
Tries to infer default <code>format</code> from the <code>outfile</code> suffix.
You can override by setting <code>format</code> explicitly.
Warns with a <code>graphviz.FormatSuffixMismatchWarning</code>
if there is a mismatch between given <code>format</code>
and the inferred format from <code>outfile</code> suffix.
Warns with a <code>graphviz.UnknownSuffixWarning</code>
if <code>format</code> is given and <code>outfile</code> uses a suffix
that cannot be mapped to a supported format.</p>
<p>Add <code>graphviz.set_jupyter_format()</code> to set the output <code>format</code>
used by the Jupyter visualization of <code>graphviz.Graph</code>, <code>graphviz.Digraph</code>,
and <code>graphviz.Source</code> (supported formats: <code>'svg'</code>, <code>'png'</code>, <code>'jpeg'</code>).
Replace <code>_repr_svg_()</code> internally with <code>_repr_mimebundle_(include, exclude)</code>
returning a mimebundle <code>{'image/svg+xml', '&lt;?xml version=...'}</code> by default.
Adds support for <code>IPython.display.display_png()</code>.
Adds support for <code>IPython.display.display_jpeg()</code>.
PR <code>[#150](xflr6/graphviz#150) &lt;https://github.com/xflr6/graphviz/pull/150&gt;</code>_ Christoph Boeddeker.</p>
<p>Add keyword-only <code>raise_if_result_exists</code> argument to <code>.render()</code>
and stand-alone <code>graphviz.render()</code>.
Raises <code>graphviz.FileExistsError</code> if the rendered file already exists.</p>
<p>Add support to for <code>.render()</code> and stand-alone <code>.render()</code>
to overwrite the input source file with the rendered output
when using the <code>outfile</code> keyword-only argument.
This probably only makes sense for text-based Graphviz formats
such as <code>dot</code> or <code>plain</code>.
You need to specify <code>overwrite_filepath=True</code> to enable this.</p>
<!-- raw HTML omitted -->
</blockquote>
<p>... (truncated)</p>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a href="https://github.com/xflr6/graphviz/commit/e3d93611046c79efad212e62f039cd261b0a16f7"><code>e3d9361</code></a> release 0.19</li>
<li><a href="https://github.com/xflr6/graphviz/commit/702b0c24324469df259e9d9cbf6e6d37b95f7f9d"><code>702b0c2</code></a> fix PyPI rendering error (?)</li>
<li><a href="https://github.com/xflr6/graphviz/commit/01a3f00bb5351de39c535a5c71ebb50a55840cbd"><code>01a3f00</code></a> fix markup</li>
<li><a href="https://github.com/xflr6/graphviz/commit/e2185cd7e3f5a644c533a9ee7f603c9375abc7f1"><code>e2185cd</code></a> reorder steps</li>
<li><a href="https://github.com/xflr6/graphviz/commit/27a3a2afd6a76f57d0a52bca578daac8adb267fc"><code>27a3a2a</code></a> fix typo</li>
<li><a href="https://github.com/xflr6/graphviz/commit/abced02611a72bccb8dccee07fd0475e576e506d"><code>abced02</code></a> move warning on empty file to expected</li>
<li><a href="https://github.com/xflr6/graphviz/commit/99b4e628ad7407b6e7bb01fb9003956a1a296006"><code>99b4e62</code></a> add pytest.deprecated_call to expected deprecation warnings in tests</li>
<li><a href="https://github.com/xflr6/graphviz/commit/13ab1d537ebf6c76f79701dcab75ba9ef2771b0c"><code>13ab1d5</code></a> fix internal deprecation warnings</li>
<li><a href="https://github.com/xflr6/graphviz/commit/683c9c3e2bb6d415bca7d780062de79a831fd979"><code>683c9c3</code></a> flake8</li>
<li><a href="https://github.com/xflr6/graphviz/commit/61ec0d8c876cc530ae8e8192c393ca844e3345a3"><code>61ec0d8</code></a> polish update-help.py</li>
<li>Additional commits viewable in <a href="https://github.com/xflr6/graphviz/compare/0.18.2...0.19">compare view</a></li>
</ul>
</details>
<br />

[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=graphviz&package-manager=pip&previous-version=0.18.2&new-version=0.19)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)

</details>
inmantaci pushed a commit to inmanta/inmanta-core that referenced this pull request Nov 29, 2021
Bumps [graphviz](https://github.com/xflr6/graphviz) from 0.18.2 to 0.19.
<details>
<summary>Changelog</summary>
<p><em>Sourced from <a href="https://github.com/xflr6/graphviz/blob/master/CHANGES.rst">graphviz's changelog</a>.</em></p>
<blockquote>
<h2>Version 0.19</h2>
<p>Add <code>PendingDeprecationWarning</code> to calls using positional arguments
that will be <strong>deprecated in a later version</strong>.
The future API will allow from one to three positional arguments
depending on the method or function.
Keyword-only arguments where not around when this library was created.
This signals dependents and in general users to start updating
or pinning to the wanted version (or range).
Crucially, this helps new users with a safer API
that allows to avoid some common mistakes.
Warnings reported in tests.</p>
<p>Add keyword-only <code>outfile</code> argument to <code>.render()</code>
and stand-alone <code>graphviz.render()</code>.
Allows to override the rendered output file name:
<code>.render(filename='spam.gv', outfile='spam.pdf')</code>
Allows to derive the <code>format</code> and the <code>filename</code>
from the rendered <code>outfile</code> name:
<code>.render(outfile='spam.svg')</code>
Tries to infer default <code>format</code> from the <code>outfile</code> suffix.
You can override by setting <code>format</code> explicitly.
Warns with a <code>graphviz.FormatSuffixMismatchWarning</code>
if there is a mismatch between given <code>format</code>
and the inferred format from <code>outfile</code> suffix.
Warns with a <code>graphviz.UnknownSuffixWarning</code>
if <code>format</code> is given and <code>outfile</code> uses a suffix
that cannot be mapped to a supported format.</p>
<p>Add <code>graphviz.set_jupyter_format()</code> to set the output <code>format</code>
used by the Jupyter visualization of <code>graphviz.Graph</code>, <code>graphviz.Digraph</code>,
and <code>graphviz.Source</code> (supported formats: <code>'svg'</code>, <code>'png'</code>, <code>'jpeg'</code>).
Replace <code>_repr_svg_()</code> internally with <code>_repr_mimebundle_(include, exclude)</code>
returning a mimebundle <code>{'image/svg+xml', '&lt;?xml version=...'}</code> by default.
Adds support for <code>IPython.display.display_png()</code>.
Adds support for <code>IPython.display.display_jpeg()</code>.
PR <code>[#150](xflr6/graphviz#150) &lt;https://github.com/xflr6/graphviz/pull/150&gt;</code>_ Christoph Boeddeker.</p>
<p>Add keyword-only <code>raise_if_result_exists</code> argument to <code>.render()</code>
and stand-alone <code>graphviz.render()</code>.
Raises <code>graphviz.FileExistsError</code> if the rendered file already exists.</p>
<p>Add support to for <code>.render()</code> and stand-alone <code>.render()</code>
to overwrite the input source file with the rendered output
when using the <code>outfile</code> keyword-only argument.
This probably only makes sense for text-based Graphviz formats
such as <code>dot</code> or <code>plain</code>.
You need to specify <code>overwrite_filepath=True</code> to enable this.</p>
<!-- raw HTML omitted -->
</blockquote>
<p>... (truncated)</p>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a href="https://github.com/xflr6/graphviz/commit/e3d93611046c79efad212e62f039cd261b0a16f7"><code>e3d9361</code></a> release 0.19</li>
<li><a href="https://github.com/xflr6/graphviz/commit/702b0c24324469df259e9d9cbf6e6d37b95f7f9d"><code>702b0c2</code></a> fix PyPI rendering error (?)</li>
<li><a href="https://github.com/xflr6/graphviz/commit/01a3f00bb5351de39c535a5c71ebb50a55840cbd"><code>01a3f00</code></a> fix markup</li>
<li><a href="https://github.com/xflr6/graphviz/commit/e2185cd7e3f5a644c533a9ee7f603c9375abc7f1"><code>e2185cd</code></a> reorder steps</li>
<li><a href="https://github.com/xflr6/graphviz/commit/27a3a2afd6a76f57d0a52bca578daac8adb267fc"><code>27a3a2a</code></a> fix typo</li>
<li><a href="https://github.com/xflr6/graphviz/commit/abced02611a72bccb8dccee07fd0475e576e506d"><code>abced02</code></a> move warning on empty file to expected</li>
<li><a href="https://github.com/xflr6/graphviz/commit/99b4e628ad7407b6e7bb01fb9003956a1a296006"><code>99b4e62</code></a> add pytest.deprecated_call to expected deprecation warnings in tests</li>
<li><a href="https://github.com/xflr6/graphviz/commit/13ab1d537ebf6c76f79701dcab75ba9ef2771b0c"><code>13ab1d5</code></a> fix internal deprecation warnings</li>
<li><a href="https://github.com/xflr6/graphviz/commit/683c9c3e2bb6d415bca7d780062de79a831fd979"><code>683c9c3</code></a> flake8</li>
<li><a href="https://github.com/xflr6/graphviz/commit/61ec0d8c876cc530ae8e8192c393ca844e3345a3"><code>61ec0d8</code></a> polish update-help.py</li>
<li>Additional commits viewable in <a href="https://github.com/xflr6/graphviz/compare/0.18.2...0.19">compare view</a></li>
</ul>
</details>
<br />

[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=graphviz&package-manager=pip&previous-version=0.18.2&new-version=0.19)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)

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

Successfully merging this pull request may close these issues.

3 participants