API consistency: switch widget #1526
Conversation
Signed-off-by: Bruno Rino <bruno.rino@gmail.com>
Signed-off-by: Bruno Rino <bruno.rino@gmail.com>
Signed-off-by: Bruno Rino <bruno.rino@gmail.com>
Signed-off-by: Bruno Rino <bruno.rino@gmail.com>
Signed-off-by: Bruno Rino <bruno.rino@gmail.com>
There was a problem hiding this comment.
For the most part, this looks great - it's all fairly mechanical renames in the demos and the backends. The only real comments I have are in the core interface - and those pretty much all relate to making it easy for the future PR that removes the backwards compatibility shim.
…at it is easier to remove Signed-off-by: Bruno Rino <bruno.rino@gmail.com>
|
It's my first time updating a PR. Let me know if I did something wrong. |
|
I'm not clear on how to move forward. Should I "Resolve Conversation", to signal that I addressed the changes requested? When these changes are OK, I'll carry on with the other changes laid out in #1521. Should I open new PR for those, or append to this PR? |
|
@bruno-rino Apologies for not responding earlier - I missed the update when you pushed the extra code. As an aside - if you've got a PR where you've made an update and you've been waiting for a review and haven't heard anything for a couple of days, don't be afraid to give a gentle nudge. Sometimes there will be a reason that we haven't responded, but more often than not, it's a simple case of missing a notification that a PR update is ready for review. Generally speaking, we prefer if the person who opens the conversation resolves it (on the basis that the person who raises a point is the person who can judge if the point has been addressed). In terms of PR process - the general guidance is "one idea per PR", and try to keep that idea as small as possible. Keeping the PRs small makes them easier to review, which means the review/merge cycle is faster, and there's less chance of a PR turning into a multi-month epic with weekly merge collisions to resolve. In this case "clean up the Switch widget" is a good target"; Something like "use |
src/core/toga/widgets/switch.py
Outdated
| enabled (bool): Whether or not interaction with the button is possible, defaults to `True`. | ||
| factory (:obj:`module`): A python module that is capable to return a | ||
| implementation of this class with the same name. (optional & normally not needed) | ||
| """ | ||
|
|
||
| def __init__( | ||
| self, | ||
| label, | ||
| text=None, # BACKWARDS COMPATIBILITY: change to positional argument when the deprecated `label` is removed |
There was a problem hiding this comment.
It took me a moment to work out what this comment was referring to; it wasn't until I stumbled over an old usage in the canvas demo that it became clear.
I can now see that this is to make toga.Switch(label='foo') legal. This is definitely needed, but we're also going to need an additional protection, because as written, toga.Switch() is legal.
src/core/toga/widgets/switch.py
Outdated
| on_change = on_toggle | ||
| # is_on replaced with value | ||
| if is_on is not None: | ||
| # Note: since `value` has a default, there is no defense against setting both `value` and `is_on` |
There was a problem hiding this comment.
We can add that defence though; you use a placeholder other than None, and check for that.
|
The test failure is caused by sphinx-doc/sphinx#10705 - a fix for that should land very soon. I'll force a re-run of the test suite once that lands. |
Codecov Report
|
The Switch widget API becomes more consistent with other widgets by renaming some properties:
The legacy API still works, with deprecation warnings. Exceptions are raised if both the new and old init parameters are used. The label text parameter, now called
text, was temporarily changed to keyword argument. It should be changed back to positional argument when the compatibility code is removed.PR Checklist: