Skip to content

Propagate signature-modifying decorators to pyplot wrappers. #15254

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 1 commit into from
Mar 22, 2020

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Sep 12, 2019

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

@anntzer anntzer force-pushed the propagate-decorators branch from b0f1695 to 144b913 Compare September 13, 2019 08:53
@anntzer anntzer force-pushed the propagate-decorators branch from 144b913 to 61a21ba Compare October 15, 2019 19:57
@anntzer anntzer force-pushed the propagate-decorators branch from 61a21ba to fbffe2f Compare December 16, 2019 12:18
@timhoffm timhoffm mentioned this pull request Jan 11, 2020
13 tasks
@anntzer anntzer force-pushed the propagate-decorators branch 3 times, most recently from e8597f7 to ce1eed0 Compare February 26, 2020 14:11
@ksunden
Copy link
Member

ksunden commented Mar 21, 2020

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.

@timhoffm
Copy link
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 anntzer force-pushed the propagate-decorators branch from ce1eed0 to 73746d6 Compare March 21, 2020 23:53
@anntzer
Copy link
Contributor Author

anntzer commented Mar 21, 2020

rebased

@QuLogic
Copy link
Member

QuLogic commented Mar 22, 2020

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

@ksunden
Copy link
Member

ksunden commented Mar 22, 2020

@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 QuLogic merged commit e92685a into matplotlib:master Mar 22, 2020
@QuLogic QuLogic added this to the v3.3.0 milestone Mar 22, 2020
@anntzer 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants