-
Notifications
You must be signed in to change notification settings - Fork 81
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
feat: trigger a calculated property on mutation #496
feat: trigger a calculated property on mutation #496
Conversation
767c9c3
to
a7fb9d4
Compare
e479327
to
9465a76
Compare
ba448f5
to
005b082
Compare
src/writer/core.py
Outdated
for p in path_list: | ||
state_proxy = self._state_proxy | ||
path_parts = p.split(".") | ||
final_handler = functools.partial(handler, self) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way of doing things is too light.
I will have to check the arguments exposed on the handler here. To be consistent, it would be necessary to inject a payload with the previous value and the current value, add the ui and add a context which indicates the mutation causing the change.
|
src/writer/core.py
Outdated
@@ -659,6 +678,48 @@ def _set_state_item(self, key: str, value: Any): | |||
self._state_proxy[key] = value | |||
|
|||
|
|||
def subscribe_mutation(self, path: Union[str, List[str]], handler: Callable[['State'], None], initial_triggered: bool = False) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to support array, the path
should be able to contain number since string can't be index
>>> obj = {"a": [1,2,3]}
>>> obj["a"]["0"]
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: list indices must be integers or slices, not str
src/writer/core.py
Outdated
|
||
for p in path_list: | ||
state_proxy = self._state_proxy | ||
path_parts = p.split(".") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't handle key containing .
.
Also, it doesn't work with Array index because the path_parts
is an array of string (see my comment below)
IMO
- it should handle a path like this one
foo["bar.baz"][0]
we might need a library to handle it - Or we should only accept array of string/number like
["foo", "bar.baz", 0]
- Or we considerate it's OK, but it might need a comment in the doc saying this limitation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
usage of .
into a state key trigger errors. That's a good point, we don't manage properly the case where a state contains .
in the key. Currently it raise many issue.
I would advocate to manage the character .
as reserved keyword
for State key. We should ignore this value and show warning when the application is loaded.
mutation of State are not monitored on list : the State
structure on the backend side supervises mutations exclusively on dictionaries, not on lists. This is an intrinsic limitation of WF.
A developer has to reassign a list to trigger the mutation. This point is covered in the Mutation detection in doc. It is therefore not possible to subscribe to a mutation on an element of a list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clear, thanks for the explanations.
About array, is it something we want to support ? Use case I see is to watch array elements in an array to get the sum. Something like this
class MyState(wf.WriterState):
arr: List[int]
total: int
def cumulative_sum(state: MyState):
state.total = sum(state.arr)
initial_state = ss.init_state({
"arr": [1,2,3,4],
"total": 0
}, schema=MyState)
initial_state.subscribe_mutation('counter', cumulative_sum)
initial_state['counter'][0] = 9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thin this question has already been debated in the past. I don't find trace about that. I think we decided that for the moment we wanted to avoid this pattern. Re-encoding a list as an observable object would have an overhead that we want to avoid.
In Python, lists are more often reconstructed rather than modified.
However, this was before the existence of typed states. Typed states allow you to specify how to behave with respect to a collection. I wonder on which usecase we would benefit from implementing this pattern.
3ae449b
to
fc0e1c5
Compare
fc0e1c5
to
99fbb72
Compare
* feat: implement subscribe_mutation
* feat: implement fixture to isolate test of global context
* feat: implement calculated properties
* docs: improve the documentation of application state
* fix: make it works on writer framework
* docs: add documentation about calculated properties
* fix: trigger initial mutation on calculated properties
* docs: document mutation event
* chore: review
* feat: subscribe_mutation supports an event handler as a function
* feat: subscribe_mutation supports an event handler as a function * feat: subscribe_mutation supports an async event handler as a function
* fix: handle dot separated expression on subscribe mutation
19c3fa9
to
c2c5822
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was a tough read but looks good to me. Should I merge?
You should |
implement FabienArcellier#37
Simple subscription
Subscription on state with strong type
Calculated property
Subscription with an event handler
In preparation of dot support expression