Skip to content

Commit d587cf7

Browse files
dustingtorresdustingtorres
committed
Code review comments.
- Use DeepHash instead of id when tracking elements - Add and cleanup unit tests - Pass in level object to compare in order to get path if desired
1 parent 409921b commit d587cf7

File tree

7 files changed

+347
-205
lines changed

7 files changed

+347
-205
lines changed

‎conftest.py

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,3 +62,21 @@ def nested_b_t2():
6262
def nested_b_result():
6363
with open(os.path.join(FIXTURES_DIR, 'nested_b_result.json')) as the_file:
6464
return json.load(the_file)
65+
66+
67+
@pytest.fixture(scope='class')
68+
def compare_func_t1():
69+
with open(os.path.join(FIXTURES_DIR, 'compare_func_t1.json')) as the_file:
70+
return json.load(the_file)
71+
72+
73+
@pytest.fixture(scope='class')
74+
def compare_func_t2():
75+
with open(os.path.join(FIXTURES_DIR, 'compare_func_t2.json')) as the_file:
76+
return json.load(the_file)
77+
78+
79+
@pytest.fixture(scope='class')
80+
def compare_func_result():
81+
with open(os.path.join(FIXTURES_DIR, 'compare_func_result.json')) as the_file:
82+
return json.load(the_file)

‎deepdiff/diff.py

Lines changed: 59 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -588,20 +588,39 @@ def _get_matching_pairs(self, level):
588588
try:
589589
matches = []
590590
y_matched = set()
591+
y_index_matched = set()
591592
for i, x in enumerate(level.t1):
592593
x_found = False
593594
for j, y in enumerate(level.t2):
594595

595-
if(self.iterable_compare_func(x, y)):
596-
y_matched.add(id(y))
596+
if(j in y_index_matched):
597+
# This ensures a one-to-one relationship of matches from t1 to t2.
598+
# If y this index in t2 has already been matched to another x
599+
# it cannot have another match, so just continue.
600+
continue
601+
602+
if(self.iterable_compare_func(x, y, level)):
603+
deep_hash = DeepHash(y,
604+
hashes=self.hashes,
605+
apply_hash=True,
606+
**self.deephash_parameters,
607+
)
608+
y_index_matched.add(j)
609+
y_matched.add(deep_hash[y])
597610
matches.append(((i, j), (x, y)))
598611
x_found = True
599612
break
600613

601614
if(not x_found):
602615
matches.append(((i, -1), (x, ListItemRemovedOrAdded)))
603616
for j, y in enumerate(level.t2):
604-
if(id(y) not in y_matched):
617+
618+
deep_hash = DeepHash(y,
619+
hashes=self.hashes,
620+
apply_hash=True,
621+
**self.deephash_parameters,
622+
)
623+
if(deep_hash[y] not in y_matched):
605624
matches.append(((-1, j), (ListItemRemovedOrAdded, y)))
606625
return matches
607626
except CannotCompare:
@@ -616,53 +635,51 @@ def _diff_iterable_in_order(self, level, parents_ids=frozenset(), _original_type
616635
else:
617636
child_relationship_class = NonSubscriptableIterableRelationship
618637

619-
620-
621638
for (i, j), (x, y) in self._get_matching_pairs(level):
622-
if self._count_diff() is StopIteration:
623-
return # pragma: no cover. This is already covered for addition.
639+
if self._count_diff() is StopIteration:
640+
return # pragma: no cover. This is already covered for addition.
624641

625-
if y is ListItemRemovedOrAdded: # item removed completely
626-
change_level = level.branch_deeper(
627-
x,
628-
notpresent,
629-
child_relationship_class=child_relationship_class,
630-
child_relationship_param=i)
631-
self._report_result('iterable_item_removed', change_level)
642+
if y is ListItemRemovedOrAdded: # item removed completely
643+
change_level = level.branch_deeper(
644+
x,
645+
notpresent,
646+
child_relationship_class=child_relationship_class,
647+
child_relationship_param=i)
648+
self._report_result('iterable_item_removed', change_level)
632649

633-
elif x is ListItemRemovedOrAdded: # new item added
634-
change_level = level.branch_deeper(
635-
notpresent,
636-
y,
637-
child_relationship_class=child_relationship_class,
638-
child_relationship_param=j)
639-
self._report_result('iterable_item_added', change_level)
650+
elif x is ListItemRemovedOrAdded: # new item added
651+
change_level = level.branch_deeper(
652+
notpresent,
653+
y,
654+
child_relationship_class=child_relationship_class,
655+
child_relationship_param=j)
656+
self._report_result('iterable_item_added', change_level)
640657

641-
else: # check if item value has changed
642-
643-
if (i != j):
644-
# Item moved
645-
change_level = level.branch_deeper(
646-
x,
647-
y,
648-
child_relationship_class=child_relationship_class,
649-
child_relationship_param=i,
650-
child_relationship_param2=j
651-
)
652-
self._report_result('iterable_item_moved', change_level)
653-
654-
item_id = id(x)
655-
if parents_ids and item_id in parents_ids:
656-
continue
657-
parents_ids_added = add_to_frozen_set(parents_ids, item_id)
658+
else: # check if item value has changed
658659

659-
# Go one level deeper
660-
next_level = level.branch_deeper(
660+
if (i != j):
661+
# Item moved
662+
change_level = level.branch_deeper(
661663
x,
662664
y,
663665
child_relationship_class=child_relationship_class,
664-
child_relationship_param=i)
665-
self._diff(next_level, parents_ids_added)
666+
child_relationship_param=i,
667+
child_relationship_param2=j
668+
)
669+
self._report_result('iterable_item_moved', change_level)
670+
671+
item_id = id(x)
672+
if parents_ids and item_id in parents_ids:
673+
continue
674+
parents_ids_added = add_to_frozen_set(parents_ids, item_id)
675+
676+
# Go one level deeper
677+
next_level = level.branch_deeper(
678+
x,
679+
y,
680+
child_relationship_class=child_relationship_class,
681+
child_relationship_param=i)
682+
self._diff(next_level, parents_ids_added)
666683

667684
def _diff_str(self, level):
668685
"""Compare strings"""

‎tests/fixtures/compare_func_result.json

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
{
2+
"dictionary_item_added": [
3+
"root['Cars'][3]['dealers']"
4+
],
5+
"dictionary_item_removed": [
6+
"root['Cars'][3]['production']"
7+
],
8+
"values_changed": {
9+
"root['Cars'][0]['dealers'][1]['quantity']": {
10+
"new_value": 50,
11+
"old_value": 20
12+
},
13+
"root['Cars'][2]['model_numbers'][2]": {
14+
"new_value": 3,
15+
"old_value": 4
16+
},
17+
"root['Cars'][3]['model']": {
18+
"new_value": "Supra",
19+
"old_value": "supra"
20+
}
21+
},
22+
"iterable_item_added": {
23+
"root['Cars'][0]['dealers'][1]": {
24+
"id": 200,
25+
"address": "200 Fake St",
26+
"quantity": 10
27+
},
28+
"root['Cars'][2]['model_numbers'][3]": 4,
29+
"root['Cars'][0]": {
30+
"id": "7",
31+
"make": "Toyota",
32+
"model": "8Runner"
33+
}
34+
},
35+
"iterable_item_removed": {
36+
"root['Cars'][0]['dealers'][0]": {
37+
"id": 103,
38+
"address": "103 Fake St",
39+
"quantity": 50
40+
},
41+
"root['Cars'][1]": {
42+
"id": "2",
43+
"make": "Toyota",
44+
"model": "Highlander",
45+
"dealers": [
46+
{
47+
"id": 123,
48+
"address": "123 Fake St",
49+
"quantity": 50
50+
},
51+
{
52+
"id": 125,
53+
"address": "125 Fake St",
54+
"quantity": 20
55+
}
56+
]
57+
}
58+
},
59+
"iterable_item_moved": {
60+
"root['Cars'][0]": {
61+
"new_path": "root['Cars'][2]",
62+
"new_value": {
63+
"id": "1",
64+
"make": "Toyota",
65+
"model": "Camry",
66+
"dealers": [
67+
{
68+
"id": 105,
69+
"address": "105 Fake St",
70+
"quantity": 50
71+
},
72+
{
73+
"id": 200,
74+
"address": "200 Fake St",
75+
"quantity": 10
76+
}
77+
]
78+
}
79+
},
80+
"root['Cars'][0]['dealers'][1]": {
81+
"new_path": "root['Cars'][0]['dealers'][0]",
82+
"new_value": {
83+
"id": 105,
84+
"address": "105 Fake St",
85+
"quantity": 50
86+
}
87+
},
88+
"root['Cars'][2]": {
89+
"new_path": "root['Cars'][1]",
90+
"new_value": {
91+
"id": "3",
92+
"make": "Toyota",
93+
"model": "4Runner",
94+
"model_numbers": [
95+
1,
96+
2,
97+
3,
98+
4
99+
]
100+
}
101+
}
102+
}
103+
}

‎tests/fixtures/compare_func_t1.json

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
{
2+
"Cars": [
3+
{
4+
"id": "1",
5+
"make": "Toyota",
6+
"model": "Camry",
7+
"dealers": [
8+
{
9+
"id": 103,
10+
"address": "103 Fake St",
11+
"quantity": 50
12+
},
13+
{
14+
"id": 105,
15+
"address": "105 Fake St",
16+
"quantity": 20
17+
}
18+
]
19+
},
20+
{
21+
"id": "2",
22+
"make": "Toyota",
23+
"model": "Highlander",
24+
"dealers": [
25+
{
26+
"id": 123,
27+
"address": "123 Fake St",
28+
"quantity": 50
29+
},
30+
{
31+
"id": 125,
32+
"address": "125 Fake St",
33+
"quantity": 20
34+
}
35+
]
36+
},
37+
{
38+
"id": "3",
39+
"make": "Toyota",
40+
"model": "4Runner",
41+
"model_numbers": [1, 2, 4]
42+
},
43+
{
44+
"id": "4",
45+
"make": "Toyota",
46+
"model": "supra",
47+
"production": false
48+
}
49+
]
50+
}

‎tests/fixtures/compare_func_t2.json

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
{
2+
"Cars": [
3+
{
4+
"id": "7",
5+
"make": "Toyota",
6+
"model": "8Runner"
7+
},
8+
{
9+
"id": "3",
10+
"make": "Toyota",
11+
"model": "4Runner",
12+
"model_numbers": [1, 2, 3, 4]
13+
},
14+
{
15+
"id": "1",
16+
"make": "Toyota",
17+
"model": "Camry",
18+
"dealers": [
19+
{
20+
"id": 105,
21+
"address": "105 Fake St",
22+
"quantity": 50
23+
},
24+
{
25+
"id": 200,
26+
"address": "200 Fake St",
27+
"quantity": 10
28+
}
29+
]
30+
},
31+
{
32+
"id": "4",
33+
"make": "Toyota",
34+
"model": "Supra",
35+
"dealers": [
36+
{
37+
"id": 123,
38+
"address": "123 Fake St",
39+
"quantity": 50
40+
},
41+
{
42+
"id": 125,
43+
"address": "125 Fake St",
44+
"quantity": 20
45+
}
46+
]
47+
}
48+
]
49+
}

0 commit comments

Comments
 (0)