making onselect a keyword argument on selectors #26000
Conversation
lib/matplotlib/widgets.py
Outdated
| @@ -3245,7 +3249,8 @@ class RectangleSelector(_SelectorWidget): | |||
| See also: :doc:`/gallery/widgets/rectangle_selector` | |||
| """ | |||
|
|
|||
| def __init__(self, ax, onselect, *, minspanx=0, minspany=0, useblit=False, | |||
| def __init__(self, ax, *, onselect=None, minspanx=0, | |||
There was a problem hiding this comment.
| def __init__(self, ax, *, onselect=None, minspanx=0, | |
| def __init__(self, ax, onselect=None, *, minspanx=0, |
The * makes it keyword only which would require deprecation etc. I think it make sense to provide a default value, but not require old code to pass onselect with a keyword. (I guess you just forgot about this line.)
|
Not really sure about the mypy tests. I think you need to update the Also, I think it would be good to add a "smoke test" for this. That is, call one of the selectors without an before that call. |
|
Oh, and reading #21929 I am not sure that this closes the issue. It is clearly a good contribution taking care of parts of it though. |
There was a problem hiding this comment.
Agree with Oscar, changing to keyword only is a breaking change.
We should either go with just giving it a default value to make it optional (my preference) or actually do the derprecation (see the _api.make_keyword_only decorator that already exists in this file).
One way or the other we need a whats new and/or API change note.
Anyone can dismiss this review.
|
Thank you for your work on this @thiagoluisbecker . Could you also add a test of creating |
|
Hi! Apologies for the delay. I understand the suggestions and I will implement them. Thanks for the answers. |
|
ping @thiagoluisbecker. Are you still planning to work on this? |
|
Hi! No. |
f88ca0d to
2526b81
Compare
|
This seems straightforward, so I've rebased, fixed the comments, added a what's new note, and modified tests to drop the Also note that there's also |
2526b81 to
04c8ca2
Compare
|
Optional: We could deprecate for kw-only use for onselect and following arguments, which would make potential later removal simpler. But that can also be done in a follow-up. |
PR summary
closes #21929
Making onselect a keyword argument on Selectors(widgets.py) with no-op functions following the suggestion made in this issue comment: #21929 (comment).
PR checklist
onselect/onmove_callbackof selectors #21929 " is in the body of the PR description to link the related issue