Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Add a make-parameter-keyword-only-with-deprecation decorator.#13601
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
I'm not sure the title of this PR/commit is the same as what it's doing. |
| @@ -242,6 +242,12 @@ def trigger_manager_draw(manager): | |||
| @staticmethod | |||
| def show(*args, **kwargs): | |||
| if args or {*kwargs} - {"block"}: | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Couldn't that be simply
def show(*args, *, block=None, **kwargs): if args or kwargs:Note also that block is not used within the method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
But it's legal to doshow(block=...); sure,block is not used but I don't want to emit a deprecation in that case as that makes it harder to write backend-independent code (or if you really want to warn, it should be something like "the block parameter has no effect in the nbagg backend").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not sure what official best practices are here, but I would make allshow() methods identical in their API. They override_Backend.show() and should formally be substitutable. Any non-working parameters should be warned about and not be ignored or rejected with a type error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh sorry, I missed that you proposed changing the signature. Yes, that works, pushed.
@QuLogic oops :) |
... and use it to make the `block` argument of plt.show() keyword only(with deprecation), with the idea of making the future signature`plt.show(figures=None, *, block=True)`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Subject to CI.
…deprecation decorator.
…601-on-v3.1.xBackport PR#13601 on branch v3.1.x (Add a make-parameter-keyword-only-with-deprecation decorator.)
(discussed in#13128 (comment))
... and use it to make the
blockargument of plt.show() keyword only(with deprecation), with the idea of making the future signature
plt.show(figures=None, *, block=True)(discussed in#13590).PR Summary
PR Checklist