Skip to content
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

Add type annotations for callbacks #2042

Open
NMertsch opened this issue Jul 22, 2023 · 2 comments
Open

Add type annotations for callbacks #2042

NMertsch opened this issue Jul 22, 2023 · 2 comments
Labels
enhancement New features, or improvements to existing features.

Comments

@NMertsch
Copy link
Contributor

What is the problem or limitation you are having?

toga.Button's on_press parameter is annotated as callable|None, but it calls this callable like f(widget), so the callable has to accept a parameter:

import toga
from toga.style import Pack
from toga.style.pack import COLUMN


class HelloWorld(toga.App):
    def startup(self) -> None:
        main_box = toga.Box(style=Pack(direction=COLUMN))

        button = toga.Button("Say Hello!", on_press=self.say_hello)
        main_box.add(button)

        self.main_window = toga.MainWindow(title=self.formal_name)
        self.main_window.content = main_box
        self.main_window.show()

    def say_hello(self) -> None:
        self.main_window.info_dialog("Hello!", "Hello!")


def main() -> None:
    return HelloWorld()

Clicking the button leads to an error, because say_hello has an incompatible signature.

Since on_press is not annotated as e.g. Callable[[Widget], None], static type checkers like mypy or PyCharm don't catch this error.

Describe the solution you'd like

I'd like on_pressed to be annotated as Callable[[Widget], Any], Callable[[Button], None], or whatever fits best and prevents such errors (I'm not deeply familiar with toga semantics).

Describe alternatives you've considered

One could also inspect the callable and only pass the widget as parameter if the signature expects a parameter.

This would also prevent runtime issues with callbacks not requiring a reference to the calling widget, which I think is the most common source for signature mismatches here.

However, a callback like show_number_dialog(x: int) -> None would still not be flagged by static type checking.

Additional context

No response

@NMertsch NMertsch added the enhancement New features, or improvements to existing features. label Jul 22, 2023
@freakboy3742
Copy link
Member

Definitely agreed that we can do a lot better than callable as an annotation for these callbacks. The key is finding a way that the annotation serves as useful documentation.

Based on in-person discussion, it sounds like defining a Protocol might be the way to go, with the added benefit that this provides a class that we can use to attach documentation to so we can describe conventions like using **kwargs on all handlers so that future changes to the handler prototype are accommodated.

NMertsch added a commit to NMertsch/toga that referenced this issue Jul 22, 2023
NMertsch added a commit to NMertsch/toga that referenced this issue Jul 23, 2023
@freakboy3742
Copy link
Member

#2044 added an initial set of these callbacks; the remaining widgets need the same pattern followed for their callbacks.

Note that as of right now, Canvas (#2029), DetailedList (#2025), Tree (#2017), Table (#2011) and OptionContainer (#1996) still have PRs in flight that alter handler definitions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New features, or improvements to existing features.
Projects
None yet
Development

No branches or pull requests

2 participants