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

state: subclass of MutableState must return _mark_dirty return value #1898

Merged
merged 2 commits into from
Oct 2, 2023

Conversation

masenf
Copy link
Collaborator

@masenf masenf commented Oct 1, 2023

Some of the methods actually return useful values, like pop, so the return value must be passed through after marking dirty.

Previous fix #1876 corrected this issue for normal event handlers using MutableProxy. This fix is needed for background tasks that use ImmutableMutableProxy.

masenf added 2 commits October 1, 2023 09:56
Some of the methods actually return useful values, like `pop`, so the return
value must be passed through after marking dirty.

Previous fix #1876 corrected this issue for normal event handlers using
MutableProxy. This fix is needed for background tasks that use
ImmutableMutableProxy.
@@ -1656,6 +1656,9 @@ async def background_task(self):
pass # update proxy instance

async with self:
# Methods on ImmutableMutableProxy should return their wrapped return value.
assert self.dict_list.pop("foo") is not None
Copy link
Collaborator

@Lendemor Lendemor Oct 2, 2023

Choose a reason for hiding this comment

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

Should we check against the actual value of foo rather than not None ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

might not hurt. i'll add it to a subsequent PR

@picklelo picklelo merged commit 4b84c2a into main Oct 2, 2023
36 checks passed
@picklelo picklelo deleted the masenf/immutable-mutable-proxy-return branch October 9, 2023 21:06
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.

3 participants