Add transaction to event slots #233
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
More robust handling of mapping between origin and its hashed value
for more information, see https://pre-commit.ci
|
Thanks for the PR, I will have a look soon. |
Comment on lines
+37
to
+38
| if getattr(self, "_origin_hash", None) is not None: | ||
| self._doc._origins.remove(cast(int, self._origin_hash)) |
There was a problem hiding this comment.
Why using getattr, since _origin_hash is set in __init__?
| @property | ||
| def origin(self) -> Any: | ||
| """The origin of the transaction.""" | ||
| return self._doc._origins.get(self._origin_hash) if self._origin_hash else None |
There was a problem hiding this comment.
Not very likely, but since a hash is an int it could be 0.
Suggested change
| return self._doc._origins.get(self._origin_hash) if self._origin_hash else None | |
| return None if self._origin_hash is None else self._doc._origins.get(self._origin_hash) |
Comment on lines
-78
to
-80
| origin_hash = self._txn.origin() | ||
| if origin_hash is not None: | ||
| del self._doc._origins[origin_hash] |
There was a problem hiding this comment.
That was a very deterministic behavior, why did you move that part to the transaction's __del__ method?
|
@fcollonval Could you explain the reasoning about adding a reference count to origins? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #232
Fixes #234
Add transaction to event slots.
This ends up a bit more complex to be able to keep a reference to the transaction in the event, this PR introduced a
BaseTransactionthat only keeps reference to the document and the origin (independently of the rust object).That ensures the introspection of the event at any time.
It also introduces a more robust way to maintain the mapping of Python origin and rust origin.