FIX: Avoid copying source script when `plot_html_show_source_link` is False in plot directive #20814

harupy · 2021-08-09T11:55:26Z

PR Summary

In the plot directive, if plot_html_show_source_link is False, it's not necessary to copy the source scripts (sorry if I'm missing something). This change slightly decreases disk usage.

PR Checklist

Has pytest style unit tests (and pytest passes).
Is Flake 8 compliant (run flake8 on changed files to check).
New features are documented, with examples if plot related.
Documentation is sphinx and numpydoc compliant (the docs should build without error).
Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

github-actions

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

tacaswell · 2021-08-09T18:32:27Z

This seems reasonable and correct to me.

Could you possibly add a few more asserts to https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/tests/test_sphinxext.py to make sure the source is and is not copied in the relevant cases?

The rst we are running against is https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/tests/tinypages/some_plots.rst

tacaswell · 2021-08-09T18:33:14Z

Thank you for working on this and welcome 😄 (sorry should have lead with that!).

harupy · 2021-08-11T13:32:28Z

@tacaswell Added a few asserts. Could you take another look?

harupy · 2021-08-11T13:32:45Z

lib/matplotlib/tests/test_sphinxext.py

+    # `plot_html_show_source_link=0` is equivalent to
+    # `plot_html_show_source_link=False`


Ref: https://www.sphinx-doc.org/en/master/man/sphinx-build.html#cmdoption-sphinx-build-D

harupy · 2021-08-11T13:33:32Z

lib/matplotlib/tests/test_sphinxext.py

+    # `plot_html_show_source_link=False`
+    build_sphinx_html(source_dir, doctree_dir, html_dir,
+                      extra_args=["-D", "plot_html_show_source_link=0"])
+    assert "some_plots-1.py" not in [p.name for p in html_dir.iterdir()]


Should I make this more strict?

tacaswell · 2021-08-11T18:42:58Z

This seems reasonable, however I have a concern that this is going to add time to the test process as tinypages was already the slowest test:

On a recent default-branch run:

https://github.com/matplotlib/matplotlib/runs/3301881275?check_suite_focus=true#step:12:149

29.61s call     lib/matplotlib/tests/test_sphinxext.py::test_tinypages

and on this PR https://github.com/matplotlib/matplotlib/pull/20814/checks?check_run_id=3301939622#step:12:149

36.72s call     lib/matplotlib/tests/test_sphinxext.py::test_tinypages

I assume we are hitting some caching in sphinx so the second run does not double the run time, but are still adding 5s (I'm a bit torn how much to trust these times though as CI runtime is notoriously variable).

I think it would be better to add another plot to the example file?

harupy · 2021-08-12T02:09:06Z

I think it would be better to add another plot to the example file?

plot_html_show_source_link is a global configuration, not a directive-level configuration, so I don't think we can do something like:

.. plot::
     :show_source_link: False
     
     # code

Can we create a directory containing an RST file, perform sphinx-build against this directory with plot_html_show_source_link =True, and then assert a source script has not been created?

Signed-off-by: harupy <hkawamura0130@gmail.com>

tacaswell · 2021-08-12T18:21:26Z

plot_html_show_source_link is a global configuration, not a directive-level configuration,

🐑 Sorry for being wrong about that and pushing you to test something impossible!

lib/matplotlib/tests/test_sphinxext.py

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>

lib/matplotlib/tests/test_sphinxext.py

harupy · 2021-08-19T02:01:39Z

@QuLogic Could you take another review?

QuLogic · 2021-08-19T07:11:22Z

Thanks @harupy! Congratulations on your first PR to Matplotlib 🎉 We hope to hear from you again.

Save script if necessary

780ea18

harupy closed this Aug 9, 2021

harupy reopened this Aug 9, 2021

github-actions bot reviewed Aug 9, 2021

View reviewed changes

QuLogic added the topic: sphinx extension label Aug 9, 2021

Add tests

b2ff453

harupy commented Aug 11, 2021

View reviewed changes

run tests faster

d1562d5

Signed-off-by: harupy <hkawamura0130@gmail.com>

tacaswell approved these changes Aug 12, 2021

View reviewed changes

QuLogic reviewed Aug 12, 2021

View reviewed changes

lib/matplotlib/tests/test_sphinxext.py Outdated Show resolved Hide resolved

lib/matplotlib/tests/test_sphinxext.py Outdated Show resolved Hide resolved

harupy and others added 2 commits August 13, 2021 09:51

Update lib/matplotlib/tests/test_sphinxext.py

55350ae

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>

Update lib/matplotlib/tests/test_sphinxext.py

de3f387

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>

harupy commented Aug 13, 2021

View reviewed changes

lib/matplotlib/tests/test_sphinxext.py Outdated Show resolved Hide resolved

Fix lint

bf0b0f5

harupy requested a review from QuLogic August 13, 2021 22:37

single quotes

515f441

QuLogic approved these changes Aug 19, 2021

View reviewed changes

QuLogic merged commit 64619e5 into matplotlib:master Aug 19, 2021

QuLogic added this to the v3.5.0 milestone Aug 19, 2021

harupy deleted the save-script-if-necessary branch August 19, 2021 07:13

StefRe mentioned this pull request Sep 12, 2022

[Doc]: Missing source files #23876

Closed

MikhailRyazanov mentioned this pull request Sep 27, 2022

[Bug]: Plot directive clutters the output directory with unneeded files #24005

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX: Avoid copying source script when `plot_html_show_source_link` is False in plot directive #20814

FIX: Avoid copying source script when `plot_html_show_source_link` is False in plot directive #20814

harupy commented Aug 9, 2021 •

edited

Loading

github-actions bot left a comment

tacaswell commented Aug 9, 2021

tacaswell commented Aug 9, 2021

harupy commented Aug 11, 2021

harupy Aug 11, 2021

harupy Aug 11, 2021

tacaswell commented Aug 11, 2021

harupy commented Aug 12, 2021 •

edited

Loading

tacaswell commented Aug 12, 2021

harupy commented Aug 19, 2021

QuLogic commented Aug 19, 2021

		# `plot_html_show_source_link=0` is equivalent to
		# `plot_html_show_source_link=False`

FIX: Avoid copying source script when plot_html_show_source_link is False in plot directive #20814

FIX: Avoid copying source script when plot_html_show_source_link is False in plot directive #20814

Conversation

harupy commented Aug 9, 2021 • edited Loading

PR Summary

PR Checklist

github-actions bot left a comment

Choose a reason for hiding this comment

tacaswell commented Aug 9, 2021

tacaswell commented Aug 9, 2021

harupy commented Aug 11, 2021

harupy Aug 11, 2021

Choose a reason for hiding this comment

harupy Aug 11, 2021

Choose a reason for hiding this comment

tacaswell commented Aug 11, 2021

harupy commented Aug 12, 2021 • edited Loading

tacaswell commented Aug 12, 2021

harupy commented Aug 19, 2021

QuLogic commented Aug 19, 2021

FIX: Avoid copying source script when `plot_html_show_source_link` is False in plot directive #20814

FIX: Avoid copying source script when `plot_html_show_source_link` is False in plot directive #20814

harupy commented Aug 9, 2021 •

edited

Loading

harupy commented Aug 12, 2021 •

edited

Loading