Skip to content

DOC: some minor fixes to the usage rewrite #21794

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

Merged
merged 3 commits into from
Nov 29, 2021

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Nov 29, 2021

PR Summary

This fixes a few typos and missed commas in usage.py.

Follow up of #21641

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • 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).
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

@jklymak jklymak added the Documentation: website layout/behavior/styling changes label Nov 29, 2021
@jklymak jklymak added this to the v3.5-doc milestone Nov 29, 2021
@dstansby
Copy link
Member

Why all the extra plt.show() calls? sphinx-gallery should show the figures under the block they were created in, and I always think it's better to just have one plt.show() call at the end when running examples locally so all the figures appear at once.

@jklymak
Copy link
Member Author

jklymak commented Nov 29, 2021

Ah, because if you don't SG put "Out: Legend(...)" in a yellow box after everything. I could just put semicolons I guess?

@dstansby
Copy link
Member

I'm +1 for semicolons as opposed to plt.show()

@jklymak
Copy link
Member Author

jklymak commented Nov 29, 2021

Hmm, except semicolons are a flake8 exception...

Copy link
Member

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

Lets leave it with plt.show()s then, not a big deal 😄

@jklymak
Copy link
Member Author

jklymak commented Nov 29, 2021

This commit drops the shows, adds ; and puts in an exception for the trailing semicolon. In exchange I got rid of the lines that were too long 😉 OTOH we never make this exception so I'm not quite sure what our policy is on it. I personally don't see what the big deal is, and it greatly enhances the readability of the examples. Maybe there is a SG setting to suppress...

@greglucas
Copy link
Contributor

I don't think we should add in the ; exception to flake8 just for this file. You could set leg = ... to capture the return, or change the capture_repr/ignore_repr
https://sphinx-gallery.github.io/stable/auto_examples/plot_3_capture_repr.html#matplotlib-output

https://sphinx-gallery.github.io/stable/configuration.html#prevent-capture-of-certain-classes

@jklymak
Copy link
Member Author

jklymak commented Nov 29, 2021

I understand in general we do not want semicolons at the end of statements, but in this case it serves a really useful purpose - it saves the extra useless and confusing output, or it saves a spurious plt.show, or it saves a spurious and confusing assignment. OTOH I won't object if someone has a way to suppress these via configuration. However, note that some of our examples want the output to show up, so it can't be global.

@jklymak jklymak mentioned this pull request Nov 29, 2021
1 task
@dstansby dstansby merged commit f091d46 into matplotlib:main Nov 29, 2021
@lumberbot-app
Copy link

lumberbot-app bot commented Nov 29, 2021

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout v3.5.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -m1 f091d46a8559178f9ddd99553bb5415476469f10
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #21794: DOC: some minor fixes to the usage rewrite'
  1. Push to a named branch:
git push YOURFORK v3.5.x:auto-backport-of-pr-21794-on-v3.5.x
  1. Create a PR against branch v3.5.x, I would have named this PR:

"Backport PR #21794 on branch v3.5.x (DOC: some minor fixes to the usage rewrite)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

@lumberbot-app
Copy link

lumberbot-app bot commented Nov 29, 2021

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout v3.5.0-doc
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -m1 f091d46a8559178f9ddd99553bb5415476469f10
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #21794: DOC: some minor fixes to the usage rewrite'
  1. Push to a named branch:
git push YOURFORK v3.5.0-doc:auto-backport-of-pr-21794-on-v3.5.0-doc
  1. Create a PR against branch v3.5.0-doc, I would have named this PR:

"Backport PR #21794 on branch v3.5.0-doc (DOC: some minor fixes to the usage rewrite)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

@tacaswell
Copy link
Member

I'm in favor of not backporting this and letting the examples start to diverge again.

@QuLogic
Copy link
Member

QuLogic commented Nov 30, 2021

This depends on #21641, which has not been backported yet. But that also conflicts, so I guess depends on whether @jklymak wants to do it.

@jklymak jklymak deleted the doc-basic-usage-fixes branch November 30, 2021 07:40
@jklymak
Copy link
Member Author

jklymak commented Nov 30, 2021

I think we should back-port these, as the current Usage Guide is very sparse....

jklymak pushed a commit to jklymak/matplotlib that referenced this pull request Nov 30, 2021
@jklymak jklymak mentioned this pull request Nov 30, 2021
6 tasks
jklymak pushed a commit to jklymak/matplotlib that referenced this pull request Nov 30, 2021
QuLogic added a commit that referenced this pull request Nov 30, 2021
dstansby added a commit that referenced this pull request Nov 30, 2021
…3.5.0-doc

Manual Backport #21794 from jklymak/doc-basic-usage-fixes
@StefRe
Copy link
Contributor

StefRe commented Dec 5, 2021

I understand in general we do not want semicolons at the end of statements, but in this case it serves a really useful purpose - it saves the extra useless and confusing output

I have no idea why, but the yellow output boxes are still present in the current devdocs Basic Usage tutorial, despite the semicolons.

@jklymak
Copy link
Member Author

jklymak commented Dec 5, 2021

Yes, that didn't work too well ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation: website layout/behavior/styling changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants