FIX: Avoid copying source script when `plot_html_show_source_link` is False in plot directive by harupy · Pull Request #20814 · matplotlib/matplotlib · GitHub

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).

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.

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

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

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

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

Should I make this more strict?

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?

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>

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!

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

@QuLogic Could you take another review?

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

Comment thread lib/matplotlib/tests/test_sphinxext.py Outdated

Comment thread lib/matplotlib/tests/test_sphinxext.py Outdated

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

Comment thread lib/matplotlib/tests/test_sphinxext.py Outdated

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

+                  # `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()]

		# `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 by harupy · Pull Request #20814 · matplotlib/matplotlib · GitHub

Repository navigation

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#20814QuLogic merged 7 commits intomatplotlib:mastermatplotlib/matplotlib:masterfrom harupy:save-script-if-necessaryharupy/matplotlib:save-script-if-necessaryCopy head branch name to clipboard

Conversation

harupy commented Aug 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Aug 9, 2021

Uh oh!

tacaswell commented Aug 9, 2021

Uh oh!

harupy commented Aug 11, 2021

Uh oh!

harupy Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

harupy Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Aug 11, 2021

Uh oh!

harupy commented Aug 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tacaswell commented Aug 12, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

harupy commented Aug 19, 2021

Uh oh!

QuLogic commented Aug 19, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

FIX: Avoid copying source script when plot_html_show_source_link is False in plot directive#20814
QuLogic merged 7 commits into
matplotlib:masterfrom
harupy:save-script-if-necessary

harupy commented Aug 9, 2021 •
edited
Loading

harupy commented Aug 12, 2021 •
edited
Loading