refactor: simplify array unset logic by removing revision lock check #37
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
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
This was referenced Jun 3, 2025
ricokahler
force-pushed
the
v6_new-dmp-behavior
branch
from
55343b7 to
23a0335
Compare
ricokahler
force-pushed
the
v6_remove-array-revision-lock-check
branch
from
2e03143 to
3aff5f8
Compare
This was referenced Jun 4, 2025
rexxars
approved these changes
Jun 6, 2025
ricokahler
force-pushed
the
v6_remove-array-revision-lock-check
branch
from
3aff5f8 to
2459585
Compare
ricokahler
force-pushed
the
v6_new-dmp-behavior
branch
from
63bee6e to
db4ab0e
Compare
Contributor
Author
Merge activity
|
Remove the isRevisionLocked() condition from array diffing logic when deciding between key-based vs index-based unset operations. The revision lock check was redundant because: - Key-based unsets are inherently safer in collaborative environments - Revision locking doesn't change the safety characteristics of the operation - If array items have unique keys, key-based operations should always be preferred regardless of whether we're targeting a specific revision - Index-based operations are still used as fallback when items lack unique keys This simplifies the logic to: if items have unique keys, always use key-based unsets; otherwise fall back to index-based unsets.
ricokahler
force-pushed
the
v6_remove-array-revision-lock-check
branch
from
2459585 to
4315ed5
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This PR refactors the
diffArrayfunction to simplify the logic for unsetting items from an array. Previously, the decision of whether to unset array items by index/range versus by_keywas partially influenced by anisRevisionLockedcheck.The
isRevisionLockedcheck (which determined ififRevisionIDwas set in options) has been removed from this specific decision point. The logic now prioritizes unsetting by_keyif the items to be unset are uniquely keyed, regardless of the revision lock status. If items are not uniquely keyed, it falls back to unsetting by index/range.Key Changes:
diffArray, whenitemB.length < itemA.length(items are removed):isRevisionLocked(options) || !isUniquelyKeyed(unsetItems)has been simplified to!isUniquelyKeyed(unsetItems).unsetItemsthemselves have unique keys. If they do, unsetting by_keyis preferred for better safety in collaborative environments.isRevisionLocked(options: PatchOptions): booleanutility function as it's no longer used in this context.Rationale:
_keyis generally safer and more robust in collaborative editing. This change makes key-based unsetting the default when applicable, promoting better conflict resolution for keyed arrays.ifRevisionIDoption's primary purpose is optimistic locking at the patch mutation level, not to dictate the internal strategy for array item unsetting. Decoupling these concerns simplifies thediffArraylogic.This refactor makes the array unsetting behavior more predictable and aligns better with the goal of generating conflict-resistant patches, especially for keyed arrays.