Repository navigation
Handle self-containing values in make_patch and add #180 regression tests - #200
Merged
Merged
Conversation
make_patch found moved values by looking up (value, type(value)) in a
dict, or with == for unhashable values. Python considers e.g. [1] and
[True], {'x': 1} and {'x': True}, or 0.0 and -0.0 equal, so a removed
value could be moved to where a different one was added, and the patch
did not produce the target document.
Move detection now keys values the way _compare_values compares them:
by their serialization with the diff's dumps, with objects and arrays
taken member by member so that the order of object members still does
not matter. Values that dumps cannot serialize are added and removed
instead of moved, so make_patch still accepts them. The key is computed
once per added or removed value, and as all keys are hashable the
linear search for unhashable values is gone.
The {'a': [1]} -> {'b': [True]} example of test_roundtrip passes now and
is dropped; the test still fails for the object member '-'.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013AREgGKdBTdMJKwCsPuhNn
The safe documents left out booleans only because make_patch confused them with numbers (#180). Now that it does not, they are generated again, and the counterexamples of both halves of #180 are pinned on test_roundtrip_of_safe_documents, as random search finds them rarely. The test for values that cannot be serialized now checks that they are removed and added, as applying a move would give the same document. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013AREgGKdBTdMJKwCsPuhNn
The keys that find moved values used member names as they are, so
{1: 'v'} and {True: 'v'} shared a key although they serialize as
{"1": "v"} and {"true": "v"}. Member names that are not strings are
serialized now.
Values that contain themselves made building their key recurse until
RecursionError, which crashed make_patch. Like other values that cannot
be serialized, they are now added and removed instead of moved.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013AREgGKdBTdMJKwCsPuhNn
master fixed the move detection of #180 on its own, with _move_key, which serializes values with their object members sorted. Its version is kept, replacing index_key and _serialized_key of this branch, and so is its behaviour for values that dumps cannot serialize: they are moved where they are added unchanged, instead of being removed and added. _move_key and _differing_runs now also treat RecursionError as a value that cannot be serialized. On master, adding or removing a value that contains itself crashed make_patch, which it did not before. The cases of test_issue180_moves are merged into master's test_move_only_values_equal_in_json. test_values_that_cannot_be_serialized is dropped, as master's test_values_dumps_cannot_serialize covers such values with the behaviour master chose. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013AREgGKdBTdMJKwCsPuhNn
stefankoegl
changed the title
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
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.
Summary
#180 has two halves, and both are now fixed on master. #181 stopped comparing list items with Python equality. #206 stopped matching moved values that way by adding
DiffBuilder._move_key. This PR started as its own fix for the second half. After merging master, it keeps master's implementation and adds a fix for values that contain themselves, plus regression tests for #180.Changes
make_patch._move_keyand_differing_runsserialize values with_sorted_members, which recurses untilRecursionErroron a value that contains itself. On master, adding or removing such a value raises that error, which did not happen before Improve list diffing to avoid unnecessary item replacements #201 and Improve list diffing with sequence alignment to avoid cascading changes #206. Both now treatRecursionErrorlike any other value thatdumpscannot serialize. Such values are moved only where the same value is added unchanged, and lists containing them are compared by position.tests.py:test_move_only_values_equal_in_jsongets more cases:{"x": 1}and{"x": true}1and1.00.0and-0.0{1: 'v'}and{True: 'v'}test_move_only_values_equal_in_given_dumps: a customdumpsdecides which values are the same, e.g.Decimal('1.0')andDecimal('1.00').test_use_move_regardless_of_member_order: a value is still moved when its object members are in a different order.test_values_that_contain_themselves: adding, removing and moving a value that contains itself.property_tests.py: random search finds the bug only rarely. So the counterexamples for both halves of Bug: jsondiff accepts list items as equal when their __eq__ returns True #180,[0] -> [False]and{'a': [1]} -> {'b': [True]}, are pinned as examples ontest_roundtrip_of_safe_documents.Testing
python tests.py,python property_tests.py(8 expected failures, as on master),python ext_tests.pyandpytest.jsonpatch.py, onlytest_values_that_contain_themselvesfails, withRecursionError.Closes #180
🤖 Generated with Claude Code
https://claude.ai/code/session_013AREgGKdBTdMJKwCsPuhNn