X Tutup
Skip to content

plot directive: caption-option#18426

Merged
QuLogic merged 6 commits intomatplotlib:masterfrom
ulijh:plot-directive-caption-option
Sep 12, 2020
Merged

plot directive: caption-option#18426
QuLogic merged 6 commits intomatplotlib:masterfrom
ulijh:plot-directive-caption-option

Conversation

@ulijh
Copy link
Contributor

@ulijh ulijh commented Sep 7, 2020

PR Summary

This PR adds an option :caption: to the plot directive. It partially solves #9346.

  • The new option enforces a caption when a single figure is generated.

  • The option take precedence over the contents when a separate file containing the code is used.

  • Tested and documented in the module's doc string.

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

plot directive: caption-option
90069b4 
This new option allows the user to add a caption to plots with the
plotting code given in the directive's contents.

* The option :caption: enforces a caption.

* The option take precedence over the contents.

* Tested and documented in the modul's doc string.

* Note in next_whats_new
@tacaswell tacaswell added this to the v3.4.0 milestone Sep 7, 2020
tacaswell
tacaswell reviewed Sep 7, 2020
View reviewed changes
timhoffm
timhoffm approved these changes Sep 7, 2020
View reviewed changes
Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

Modulo raising on ambiguity.

@ulijh
Copy link
Contributor Author

ulijh commented Sep 8, 2020

@tacaswell @timhoffm after I submitted I found out there was another PR #15304 doing the same thing. Could you have a look and check which one should be taken forward? Thanks a bunch.

@jklymak
Copy link
Member

jklymak commented Sep 8, 2020

@ulijh, thanks for finding that. I would tend to say luck of the draw on this one. The original PR author probably needed to nag us a bit - if you are hung ho to see this through lets stick w/ this PR.

@ulijh
Copy link
Contributor Author

ulijh commented Sep 8, 2020

I've just changed this to raise if the caption is given twice.

Two things I could need some help with:

  1. How to get the rst source line into the error message, and

  2. how to test this properly. Creating a whole new tinypages for this seems a bit over the top. May be the tests in Sphinx extension: support captions in inline plots. #15304 can help with that? If so, may be we should merge this PRs. @bcbnz what do you think?

QuLogic
QuLogic reviewed Sep 9, 2020
View reviewed changes
Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

Some small typos.

I'm not sure we have any way to test failure cases for the Sphinx extension.

@QuLogic
Copy link
Member

QuLogic commented Sep 9, 2020 •
edited
Loading

You can add proper context using self.error. However, the way we set up things to support both the class-based Directive and standalone function means you don't have self.

To support both still, I think you'd have to add a try/except Exception as e around here:

matplotlib/lib/matplotlib/sphinxext/plot_directive.py

Lines 245 to 248 in 855e3bb

def run(self):
"""Run the plot directive."""
return run(self.arguments, self.content, self.options,
self.state_machine, self.state, self.lineno)

and then call self.error(str(e))

ulijh and others added 4 commits September 9, 2020 07:38
@ulijh @QuLogic
Update lib/matplotlib/sphinxext/plot_directive.py
bda148e 
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
@ulijh @QuLogic
Update lib/matplotlib/tests/tinypages/some_plots.rst
9107c12 
Fix typo in sphinx test document.

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
@ulijh @QuLogic
Update lib/matplotlib/tests/test_sphinxext.py
1d6e939 
Fix typo in comment.

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
Using self.error in drective to provide context.
c6ab4ab
@ulijh
Copy link
Contributor Author

ulijh commented Sep 11, 2020

Hey guys, the context is added when the error is raised. From my point of view this seems OK to merge. Thanks to the reviewers!

@QuLogic QuLogic merged commit 33affe1 into matplotlib:master Sep 12, 2020
@QuLogic
Copy link
Member

QuLogic commented Sep 12, 2020

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

@dopplershift dopplershift mentioned this pull request Sep 12, 2020
Closed
6 tasks
@ulijh ulijh deleted the plot-directive-caption-option branch September 16, 2020 09:25
@ulijh
Copy link
Contributor Author

ulijh commented Sep 16, 2020

Thanks for merging! :-) I'm very happy to have contributed (a little bit) to this great project! Thanks to all who invest their time in it!

@QuLogic QuLogic mentioned this pull request Mar 18, 2021
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tacaswell tacaswell tacaswell approved these changes

@QuLogic QuLogic QuLogic left review comments

@timhoffm timhoffm timhoffm approved these changes

Assignees

No one assigned

Labels

topic: sphinx extension

Projects

None yet

Milestone

v3.4.0

Development

Successfully merging this pull request may close these issues.

5 participants

@ulijh @jklymak @QuLogic @tacaswell @timhoffm
X Tutup