FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Propagate signature-modifying decorators to pyplot wrappers. by anntzer · Pull Request #15254 · matplotlib/matplotlib · GitHub

Repository navigation

Propagate signature-modifying decorators to pyplot wrappers. - #15254

Merged
QuLogic merged 1 commit into
matplotlib:masterfrom
anntzer:propagate-decorators
Mar 22, 2020
Merged

QuLogic merged 1 commit into
matplotlib:masterfrom
anntzer:propagate-decorators

Conversation

anntzer commented Sep 12, 2019

Copy link
Copy Markdown
Contributor

PR Summary

alternative to #14130, see discussion there; attn @timhoffm

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

Comment thread lib/matplotlib/tests/test_pyplot.py Outdated
anntzer force-pushed the propagate-decorators branch from b0f1695 to 144b913 Compare September 13, 2019 08:53
anntzer force-pushed the propagate-decorators branch from 144b913 to 61a21ba Compare October 15, 2019 19:57
anntzer force-pushed the propagate-decorators branch from 61a21ba to fbffe2f Compare December 16, 2019 12:18
timhoffm mentioned this pull request Jan 11, 2020
8 of 13 tasks
anntzer force-pushed the propagate-decorators branch 3 times, most recently from e8597f7 to ce1eed0 Compare February 26, 2020 14:11

ksunden commented Mar 21, 2020

Copy link
Copy Markdown
Member

What is the disposition on this PR (asking in relation to #15049)? Looks like the conflict is in generated code, and can be resolved rather simply.

Copy link
Copy Markdown
Member

There's no final decision between this and #14130 yet. In the end, I don't have a very strong opinion anymore on which one to choose. The important thing is that we get one of them in, so that we can properly rename parameters and propagate that to pyplot. Since this is internal functionality, we could even switch out the solution if we realize the other one was better.

In practice, we need a second positive review so that this can get merged. The confict is indeed trivial to solve by regenerating the code.

anntzer force-pushed the propagate-decorators branch from ce1eed0 to 73746d6 Compare March 21, 2020 23:53

anntzer commented Mar 21, 2020

Copy link
Copy Markdown
Contributor Author

rebased

QuLogic commented Mar 22, 2020

Copy link
Copy Markdown
Member

I want to merge, but I'm wondering if there's an example that would trigger this yet?

ksunden commented Mar 22, 2020

Copy link
Copy Markdown
Member

@QuLogic I think there are others, but one such example is the s argument to plt.annotate being changed to text (See #15049, as that change is also required for this example)

The argument (and docstring) were changed in the actual implementation, but pyplot still has the old kwarg (with the new docstring)

QuLogic merged commit e92685a into matplotlib:master Mar 22, 2020
QuLogic added this to the v3.3.0 milestone Mar 22, 2020
QuLogic added the API: argument checking Validation of acceptable argument values label Mar 22, 2020
anntzer deleted the propagate-decorators branch March 22, 2020 09:52
ksunden added a commit to ksunden/matplotlib that referenced this pull request Mar 26, 2020
Remove specification for where the old parameter remains supported

This was fixed by merging matplotlib#15254
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

API: argument checking Validation of acceptable argument values

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants


Back | FazBrowse Home | New Git URL