Repository navigation
Preserve dictionary key casing in case-insensitive diff paths - #630
saitakarcesme wants to merge 2 commits into
Conversation
seperman
left a comment
There was a problem hiding this comment.
Static review: #630 Preserve dictionary key casing in case-insensitive diff paths
Summary: For dict keys present in both inputs, _diff_dict now builds the child relationship from the original keys (key1 for t1, key2 for t2) instead of the lowercased match key, when ignore_string_case=True and both keys are str. Reported paths keep the input's casing, and path(use_t2=True) gives t2's spelling. As far as I can tell from reading the code, this fixes #515 for string keys, including exclude_paths/exclude_regex_paths written in the original casing.
Overlap with other PRs
- No other PR targets #515. Related but different: #640 (bytes-key path parsing in
path.py) and #635 (temporal keys innumber_to_string). Neither touches this hunk. - Textual conflicts: only
CHANGELOG.md, with #620 and #618 (trivial). - Semantic interaction with #634. #634's threshold pre-check builds intersecting-key paths from the normalized key (
path_key = keyin itsdiff.py:773), which matchesdevtoday. After this PR the real path useskey1, so whichever lands second should align that line (e.g.t1_clean_to_keys[key] if t1_clean_to_keys else key).
Security
No security concerns found in static review.
Correctness
deepdiff/diff.py:817-824: thekey1/key2lookups already exist and are used to fetch the values, so only the relationship params change. Added and removed keys were already reported with original keys (diff.py:781/795), so all three branches are now consistent.- Visible side effects worth noting in the CHANGELOG, both of which I think are improvements:
- Paths for intersecting keys change from lowercase to the original casing.
- Because
path() != path(use_t2=True)when the casing differs,verbose_level=2output and the Delta view now include anew_pathentry for thesevalues_changed/type_changes(model.py:209-212,:222-225,:396-400,:412-415).Deltaapplies forward changes atpath(t1 casing) and reverse atnew_path(t2 casing). That should make deltas from case-insensitive diffs apply to the real keys, where before they used the lowercased ones.
- Question, possibly for a follow-up: why limit this to
str/strpairs? From reading_get_clean_to_keys_mapping(diff.py:672-711), whenever a key-cleaning flag is set, numeric keys are normalized to strings likeint:1ornumber:1.000000000000. SoDeepDiff({1: 'a'}, {1: 'b'}, ignore_string_case=True)would report underroot['int:1'], both before and after this PR; I have not run this. Usingkey1/key2unconditionally for intersecting keys looks like it would fix that case as well, and it matches what the added/removed loops already do. Thegroup_by_keysspecial case in that function suggests this has come up before. - The threshold pre-check for
exclude_paths(diff.py:764) still formats the normalized key. Withignore_string_case=True, exactexclude_pathsin the original casing therefore still aren't recognised there, e.g.{'Foo1': 0}vs{'Foo2': 0}with both paths excluded still reportsroot. This is not introduced here, but the CHANGELOG wording ("so path exclusions can use the input keys") might be slightly broader than what's covered.
Tests
tests/test_diff_text.py:372,:385and:393would fail ondev, which reportsroot['user']['aboutme'], or does not exclude it. They coverpath(),path(use_t2=True), exact and regex exclusion, and nested added/removed keys.- Suggest adding a
verbose_level=2(or Delta round-trip) case to pin down the newnew_pathbehaviour. - No CI run is shown for this branch.
AI-assisted static review; the code was not executed.
|
Thanks for the detailed review. Updated in 9ec4391:
All four new cases fail with the unchanged I also reproduced both observations about the existing behavior: numeric keys can still be reported as #634 is still open. The integration point you identified remains relevant: its intersecting-key threshold paths should use the original t1 string key when this fix is present. No changes from that separate PR are included here. The new upstream CI run still requires maintainer approval; could you approve it when reviewing this update? Implementation, test execution and this follow-up were prepared with Codex assistance. |
With
ignore_string_case=True, comparing{"User": {"AboutMe": 1}}and{"user": {"aboutme": 2}}reports a path using lowercase keys that do not exist in the first input.exclude_pathsandexclude_regex_pathsusing the original keys consequently fail to exclude the change.Keep case-normalized keys for matching, but use each input's original string key for its tree relationship. This preserves case-insensitive key matching and exposes the second input's spelling through
path(use_t2=True). Numeric and bytes key normalization retain their existing behavior. Reported paths now use original casing, so callers depending on lowercase output paths should update their expectations. Atverbose_level=2, value and type changes include the second input's spelling innew_path. Bidirectional Delta application uses each input's actual paths without renaming keys whose casing is ignored. The existingthreshold_to_diff_deepershortcut is unchanged; exclusions using original paths apply when comparison reaches the matched keys.Fixes #515.
Validation:
devand pass with this PR.git diff --checkpasses; added test lines have no flake8 findings.Disclosure: implemented and tested using OpenAI Codex in an authorized automated contribution session.