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

Decorator madness #4205

Open
wants to merge 29 commits into
base: main
Choose a base branch
from
Open

Decorator madness #4205

wants to merge 29 commits into from

Conversation

willmcgugan
Copy link
Collaborator

@willmcgugan willmcgugan commented Feb 24, 2024

Decorators for watch, compute, and validate. Works like this:

class MyWidget(Widget):

    count = reactive(0)
    double_count = reactive(0)

    @count.watch
    def count_changed(self, new_count:int):
        ...

    @count.validate
    def _max_ten(self, value:int) -> int:
        return min(10, value)

    @double_count.compute
    def _double(self) -> int:
        return self.count * 2

@willmcgugan willmcgugan marked this pull request as draft February 24, 2024 15:29
@willmcgugan willmcgugan marked this pull request as ready for review February 27, 2024 15:25
Copy link
Contributor

@rodrigogiraoserrao rodrigogiraoserrao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm excited for these changes but I think there are a couple of loose ends we must tie before merging.

docs/guide/reactivity.md Outdated Show resolved Hide resolved
src/textual/reactive.py Outdated Show resolved Hide resolved
src/textual/reactive.py Outdated Show resolved Hide resolved
src/textual/reactive.py Outdated Show resolved Hide resolved
src/textual/reactive.py Outdated Show resolved Hide resolved
src/textual/reactive.py Outdated Show resolved Hide resolved
src/textual/reactive.py Outdated Show resolved Hide resolved
src/textual/reactive.py Show resolved Hide resolved
src/textual/reactive.py Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're missing, at least, a test that makes sure that init=False in the watch decorator is respected.

willmcgugan and others added 3 commits February 28, 2024 08:22
Co-authored-by: Rodrigo Girão Serrão <5621605+rodrigogiraoserrao@users.noreply.github.com>
Co-authored-by: Rodrigo Girão Serrão <5621605+rodrigogiraoserrao@users.noreply.github.com>
Co-authored-by: Rodrigo Girão Serrão <5621605+rodrigogiraoserrao@users.noreply.github.com>
willmcgugan and others added 5 commits February 28, 2024 08:22
Co-authored-by: Rodrigo Girão Serrão <5621605+rodrigogiraoserrao@users.noreply.github.com>
Co-authored-by: Rodrigo Girão Serrão <5621605+rodrigogiraoserrao@users.noreply.github.com>
Co-authored-by: Rodrigo Girão Serrão <5621605+rodrigogiraoserrao@users.noreply.github.com>
Co-authored-by: Rodrigo Girão Serrão <5621605+rodrigogiraoserrao@users.noreply.github.com>
@@ -163,6 +163,31 @@ A common use for this is to restrict numbers to a given range. The following exa

If you click the buttons in the above example it will show the current count. When `self.count` is modified in the button handler, Textual runs `validate_count` which performs the validation to limit the value of count.

### Validate decorator

In addition to the the naming convention, you can also define a validate method via a decorator.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
In addition to the the naming convention, you can also define a validate method via a decorator.
In addition to the naming convention, you can also define a validate method via a decorator.

@@ -311,15 +354,15 @@ Compute methods are the final superpower offered by the `reactive` descriptor. T

You could be forgiven in thinking this sounds a lot like Python's property decorator. The difference is that Textual will cache the value of compute methods, and update them when any other reactive attribute changes.

The following example uses a computed attribute. It displays three inputs for each color component (red, green, and blue). If you enter numbers in to these inputs, the background color of another widget changes.
The following example uses a computed attribute. It displays three inputs for each color component (red, green, and blue). If you enter numbers into these inputs, the background color of another widget changes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The following example uses a computed attribute. It displays three inputs for each color component (red, green, and blue). If you enter numbers into these inputs, the background color of another widget changes.
The following example uses a computed attribute. It displays one input for each color component (red, green, and blue). If you enter numbers into these inputs, the background color of another widget changes.
Suggested change
The following example uses a computed attribute. It displays three inputs for each color component (red, green, and blue). If you enter numbers into these inputs, the background color of another widget changes.
The following example uses a computed attribute. It displays three inputs, one for each color component (red, green, and blue). If you enter numbers into these inputs, the background color of another widget changes.

### Compute decorator

Compute methods may also be defined by the `compute` decorator on reactives.
The following examples replaces the naming convention with an equivalent decorator:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The following examples replaces the naming convention with an equivalent decorator:
The following example replaces the naming convention with an equivalent decorator:

@@ -50,6 +53,108 @@ class TooManyComputesError(ReactiveError):
"""Raised when an attribute has public and private compute methods."""


class WatchDecorator(Generic[WatchMethodType]):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I do have a suggestion.
The decorators (Watch / Compute / Validate) shouldn't be generic over the type of the method they're decorating; rather, they should be generic over the type of the reactive involved because that's what can really change.

For example, for my compute methods, I know I'll always be decorating methods with type Callable[[Any], ReactiveType].
So, what maybe I can have is an alias for compute methods and then use that.

The suggestion below seems to eliminate an error you get with make typecheck and it also gets rid of some IDE errors I get in several places.
However, I'm not 100% sure this is better.
It feels better, but I wouldn't bet my life on it.

# ...

ComputeMethodAlias: TypeAlias = Callable[[Any], ReactiveType]  # <--

# ...

class ComputeDecorator(Generic[ReactiveType]):  # <--
    def __init__(self, reactive: Reactive[ReactiveType] | None = None) -> None:  # <-- better typing here
        self._reactive = reactive

    @overload
    def __call__(self) -> ComputeDecorator[ReactiveType]: ...  # <--

    @overload
    def __call__(
        self, method: ComputeMethodAlias[ReactiveType]  # <--
    ) -> ComputeMethodAlias[ReactiveType]: ...  # <--

    def __call__(
        self, method: ComputeMethodAlias[ReactiveType] | None = None  # <--
    ) -> ComputeMethodAlias[ReactiveType] | ComputeDecorator[ReactiveType]:  # <--
        # ...

# ...

@rich.repr.auto
class Reactive(Generic[ReactiveType]):
    # ...
    def __init__(...):
        # ...
        self._compute_method: ComputeMethodAlias[ReactiveType] | None = None  # <--  better typing

    # ...

    @property
    def compute(self) -> ComputeDecorator[ReactiveType]:  # <--
        # ...


watcher_call_count = 0

@count.watch()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was very difficult for me to spot the difference.
Would it be worth adding a comment # <-- to make the diff obvious so that no one deletes this test in the future by mistake?



async def test_watch_decorator_init_true():
"""Test watchers defined via a decorator, that is called."""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"""Test watchers defined via a decorator, that is called."""
"""Test watchers defined via a decorator, that is called, when the reactive has init set."""

@darrenburns
Copy link
Member

With this change, how would we override what a watcher does in a widget subclass?

If I have a count reactive declared in a widget which has a watcher defined, then I subclass that and I want my own watcher, how would I achieve this? In the child class, @count.watch doesn't work because count is not defined.

For example, this fails to run because NameError: name 'count' is not defined

    class Parent(Widget):
        count: Reactive[int] = reactive(0, init=False)

        @count.watch
        def _count_parent(self, new_value: int) -> None:
            print("parent watcher")

    class Child(Parent):

        @count.watch
        def _count_parent(self, new_value: int) -> None:
            print("child watcher")

@Parent.count.watch doesn't work because of the "only a single method may be decorated with watch".

Even if that's solved, there's still a bit of a developer experience issue that @davep mentioned, whereby we no longer know what the parent watch method is called without going looking for it.

@willmcgugan
Copy link
Collaborator Author

Going to park this for now. @darrenburns 's observation re inheritance may kill this idea entirely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants