Skip to content

Preserve dictionary key casing in case-insensitive diff paths - #630

Open
saitakarcesme wants to merge 1 commit into
qlustered:devfrom
saitakarcesme:codex/preserve-dictionary-key-casing
Open

saitakarcesme wants to merge 1 commit into
qlustered:devfrom
saitakarcesme:codex/preserve-dictionary-key-casing

Conversation

@saitakarcesme

Copy link
Copy Markdown

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_paths and exclude_regex_paths using 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.

Fixes #515.

Validation: the original-path and exclusion regressions failed before the fix. Full suite: 1,276 passed, 44 skipped, one pre-existing macOS resource.setrlimit(RLIMIT_AS) failure reproduced on the unchanged base. Pyright's 50 existing diagnostics and flake8 diagnostics match the unchanged base after accounting for line shifts. git diff --check passes.

Disclosure: implemented and tested using OpenAI Codex in an authorized automated contribution session.

@seperman seperman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 in number_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 = key in its diff.py:773), which matches dev today. After this PR the real path uses key1, 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: the key1/key2 lookups 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=2 output and the Delta view now include a new_path entry for these values_changed/type_changes (model.py:209-212, :222-225, :396-400, :412-415). Delta applies forward changes at path (t1 casing) and reverse at new_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/str pairs? From reading _get_clean_to_keys_mapping (diff.py:672-711), whenever a key-cleaning flag is set, numeric keys are normalized to strings like int:1 or number:1.000000000000. So DeepDiff({1: 'a'}, {1: 'b'}, ignore_string_case=True) would report under root['int:1'], both before and after this PR; I have not run this. Using key1/key2 unconditionally for intersecting keys looks like it would fix that case as well, and it matches what the added/removed loops already do. The group_by_keys special 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. With ignore_string_case=True, exact exclude_paths in the original casing therefore still aren't recognised there, e.g. {'Foo1': 0} vs {'Foo2': 0} with both paths excluded still reports root. 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, :385 and :393 would fail on dev, which reports root['user']['aboutme'], or does not exclude it. They cover path(), 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 new new_path behaviour.
  • No CI run is shown for this branch.

AI-assisted static review; the code was not executed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants