fix(state): ensure change listeners are triggered in modification order#83
Conversation
|
Small comment on the copy of the key for the iterator to allow modification while iterating. |
| self._currents.clear() | ||
|
|
||
| def __iter__(self): | ||
| return iter(list(self._currents)) |
There was a problem hiding this comment.
The list was there to make a copy and allow internal update while iterating.
There was a problem hiding this comment.
Thx for the review! I have rollbacked this change.
After introducing the _suppress_change_stack this copy is handled by the following code which returns the current listeners to trigger and clears the stack to properly check for newly modified keys:
self._state_listeners.add_all(
self._suppress_change_stack.get_change_listener_keys()
)
# Clear change keys before triggering listeners as listeners can trigger modifications in chain
self._suppress_change_stack.clear()This reentrant case is correctly covered by the test_minimum_change_detection test case.
Thibault-Pelletier
force-pushed
the
fix-change-listener-order
branch
from
0ab35d3 to
a19ebef
Compare
Update StateChangeHandler and _SuppressListenersChangeStack to use an ordered set instead of a set to ensure listeners are triggered in the same order when flushed
Thibault-Pelletier
force-pushed
the
fix-change-listener-order
branch
from
a19ebef to
38dc9b0
Compare
Install package in editable mode to properly map pytest results and src folder
|
I have rebased this PR on main and added another fix for the coverage which was still 0% after my previous CI fix. I hadn't seen that the CI also reported the coverage for PR and coverage was still 0% ... |
| - name: Install and Run Tests | ||
| run: | | ||
| pip install ".[dev]" | ||
| pip install -e ".[dev]" |
There was a problem hiding this comment.
If you need -e that probably mean we are not properly installing something?
There was a problem hiding this comment.
The -e is only needed so that the coverage reports outputs something more than 0% since moving the files to src.
My previous fix of the src path for the coverage didn't resolve the problem but installing using -e did.
Locally I can also run the tests without installing and providing the PYTHON_PATH with ./src to get coverage as well so I suppose the problem is coming from where the coverage expects the files to be.
I'm open to suggestions on this!
There was a problem hiding this comment.
The only thing that I'm saying is that if we mess up something on the install part, we could miss it because of that. But I'm ok to merge for now.
2 participants
Update StateChangeHandler and _SuppressListenersChangeStack to use an ordered set instead of a set to ensure listeners are triggered in the same order when flushed