External watch fix init #4030
External watch fix init #4030
Conversation
Typing reported (correctly) that the membership check would never evaluate to 'True' because we were comparing apples with tuples of oranges and apples. 'watcher_list' contains tuples whose second element _might_ match the callback, so we need to go over the tuples and unpack them to figure out if the callback is there.
When programmatically creating a watcher to a reactive attribute, init only the new watcher instead of triggering all watchers. Related issue: #3878
| if callback in watcher_list: | ||
| if any(callback == callback_from_list for _, callback_from_list in watcher_list): |
There was a problem hiding this comment.
This was checking if it could find an apple in a list of tuples of oranges and apples.
Instead, we must unpack the tuples in the list and compare apples to apples.
|
@rodrigogiraoserrao Please follow up on this. Please also update the description to summarize your changes. |
tests/test_reactive.py
Outdated
| assert logs == ["test_1", "test_2_extra"] | ||
| app.query_one(SomeWidget).test_2 = 73 | ||
| assert logs.count("test_2_extra") == 2 | ||
| assert "test_2" in logs |
There was a problem hiding this comment.
Let's make sure that the watcher fires exactly once instead of at least once.
| assert "test_2" in logs | |
| assert logs.count("test_2") == 1 |
There was a problem hiding this comment.
What are the ordering guarantees here actually? Do we need to ensure that the new test_2_extra comes before/after the new test_2 in the logs?
There was a problem hiding this comment.
I don't remember ever seeing that we must call watchers in any specific order. Do we..?
There was a problem hiding this comment.
I don't know that it's documented, but we should ensure it's consistent I suppose 🤷
tests/test_reactive.py
Outdated
|
|
||
| app = MyApp() | ||
| async with app.run_test(): | ||
| before = counter |
There was a problem hiding this comment.
Any reason why you do the before and after stuff here instead of assert counter == 1 before the assignment and then assert counter == 2 after the assignment? That would seem simpler and a more accurate test.
As it stands, this test wouldn't catch duplicate watchers firing on init.
There was a problem hiding this comment.
I was testing whether the assignment triggered a single watcher but I agree your suggestion yields a more accurate test.
There was a problem hiding this comment.
See 1e682c2 for the change you suggested + a fix to a bug that your suggestion uncovered.
Co-authored-by: Darren Burns <darrenburns@users.noreply.github.com>
Review comment: #4030 (comment)
| self.holder = Holder() | ||
|
|
||
| def on_mount(self) -> None: | ||
| self.watch(self.holder, "attr", self.callback) |
There was a problem hiding this comment.
The test name/docstring says it's checking that we skip duplicate watchers, but the test no longer involves a duplicate watcher. It seems to just be checking the basic usage of watch, which is probably covered by another test.
I reckon this should be updated such that are two watchers defined with self.watch(self.holder, "attr", self.callback), and checking that the self.callback fires only once on init, and once again on setting the value of attr.
Perhaps more importantly, another test should probably cover this case: If I define one watcher as self.watch(self.holder, "attr", self.first_callback) and then another with self.watch(self.holder, "attr", self.second_callback). Do I see one run of self.first_callback followed by one run of self.second_callback on init? Then when I assign attr, do I see the another one run of each callback in the correct order?
There was a problem hiding this comment.
As for the first two paragraphs, I think I deleted the second callback when I was debugging the bug I mentioned in a previous commit and then forgot to add it again, thanks for spotting that...
As for the third paragraph... Do you mean something like the test test_external_watch_init_does_not_propagate_to_externals added in 8b349e3?
See first two paragraphs of #4030 (comment).
See third paragraph of #4030 (comment).
Fixes #3878
When a new watcher is created, we invoke that single watcher instead of checking all of the reactive watchers.
To enable this, we extract two auxiliary functions to the module scope from the inner scope they were defined in.