DiffBuilder: fix operation reindexing #173
Open
Conversation
This file contains hidden or 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
When an AddOperation would match a prior RemoveOperation, a MoveOperation is inserted instead, and the RemoveOperation is deleted. But when the RemoveOperation is deleted, subsequent operations that depended on it are supposed to have their paths adjusted by _on_undo_remove. Previously, _on_undo_remove was guarded by type(key) == int, which fails to consider cases where the key is a string but the operation path still needs to be adjusted. The correct guard should be type(op.pointer.to_last(self.dst_doc)[0]) == list, because _on_undo_remove should apply whenever the item removed was contained in a list.
This grants flexility for subclassing. Also removed a comment that previously existed because of discrepancy between _item_added and _item_removed that no longer exists.
angela-tarantula
changed the title
angela-tarantula
changed the title
|
@stefankoegl ping |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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 #160
TL;DR
DiffBuilderprocesses operations sequentially and greedily generates Move operations when it detects that a current Remove (or Add) corresponds to a matching Add (or Remove) that occurred earlier in the sequence. For example, if a Remove operation is encountered and a matching Add is found several steps back,DiffBuilderreplaces the two with a Move operation. When this happens, the intervening operations, those between the original Add and the new Move, must be reindexed if their'from'or'to'depended on the removed operation.In this issue, a Move operation wasn’t being reindexed because
_item_addedincorrectly gated reindexing behind the conditionif type(op.key) == int and type(key) == int.What changes
_item_addedand_item_removed, determine the parent collection usingparent_collection = op.pointer.to_last(self.dst_doc)[0].if isinstance(parent_collection, MutableSequence). This preserves the original intent but is more accurate and supports subclasses, avoiding issues with numeric-string dictionary keys or mixed structures.added_itemtoparent_collectionfor improved clarity and correctness.Tests
test_issue_160: confirms that removing an operation targeting a list correctly triggers_on_undo_addand produces the expected final document.