making onselect a keyword argument on selectors#26000
Conversation
| """ | ||
|
|
||
| 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. |
QuLogic
force-pushed
the
onselect_noop
branch
from
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 |
QuLogic
force-pushed
the
onselect_noop
branch
from
2526b81 to
04c8ca2
Compare
timhoffm
left a comment
timhoffm
left a comment
There was a problem hiding this comment.
This is a good step forward and does not bar later removal. In fact, it makes it slightly simpler since people can from now on leave out onselect if they don't need it, which will make their code forward-compatible with a possible future removal.
|
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. |
8 participants
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