Skip to content

Show removed keys in colored view when a dict becomes empty - #621

Open
koushik777-ML wants to merge 1 commit into
qlustered:devfrom
koushik777-ML:devin/1787814591-colored-view-empty-dict
Open

koushik777-ML wants to merge 1 commit into
qlustered:devfrom
koushik777-ML:devin/1787814591-colored-view-empty-dict

Conversation

@koushik777-ML

Copy link
Copy Markdown

Problem

While investigating issue #575, I found a related edge case in ColoredView when a dictionary becomes empty after keys are removed.

In this case, the colored view could return before the removed-key handling was processed, resulting in the removed keys not being displayed correctly.

The original list/empty-list behavior reported in #575 is already fixed on the current dev branch, so this PR addresses the related dictionary case.

Changes

Fix the early-return behavior in ColoredView
Ensure removed dictionary keys are shown when the dictionary becomes empty
Add regression tests covering the edge case

Testing

Added regression tests for the dictionary case
Full test suite: 1277 passed, 44 skipped

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>

@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: #621 Show removed keys in colored view when a dict becomes empty

Summary: In ColoredView._colorize_json, the dict branch now collects removed keys before the empty-dict early return and returns '{}' only when nothing was removed under that path. This mirrors the list-branch fix already on dev, so a dict that ends up empty now shows its removed keys in red instead of a bare {}. On origin/dev, the dict branch still has if not obj: return '{}' before the removed-key handling, so the bug is real.

Overlap with other PRs

  • #573 (merged into dev) fixed only the list branch for #575.
  • #578 (closed) contained the same dict-branch change as this PR. The maintainer closed it as already resolved and copied its tests to dev. However, the copied dict test, test_colored_view_with_empty_dict_shows_removals (tests/test_colored_view.py:542), uses a two-key dict. With the default threshold_to_diff_deeper=0.33, _diff_dict reports that case as a single root-level values_changed, so the test passes without touching the dict-removal branch.
  • #586 (closed) was list-only.

So this PR supplies the dict half of #578 that never landed. I found no other open PR touching colored_view.py.

Security

No security concerns found in static review. The PR doesn't change how keys and values are escaped or formatted. ANSI codes are still emitted only from the existing constants.

Correctness

  • deepdiff/colored_view.py:96-98, 107: the logic is correct. _get_path_removed is now called once per dict instead of after the item loop, so non-empty dicts do the same amount of work. Empty dicts add one scan of diff_paths, which is negligible.
  • Scope note: with the default threshold_to_diff_deeper=0.33, _diff_dict emits per-key dictionary_item_removed entries for an emptied dict only when the original dict had a single key (the t_keys_union_len > 1 guard in deepdiff/diff.py). With two or more keys the intersection ratio is 0, so the whole dict is reported as values_changed and rendered as old -> new, which was already readable. By default, then, this fix affects dicts that had one key, or dicts of any size when threshold_to_diff_deeper=0.

Tests

  • tests/test_colored_view.py:167-211: the three removal tests would fail on origin/dev, which renders {}. They pass with the change and exercise the dict-removal branch directly, including nesting and COLORED_COMPACT_VIEW. test_colored_view_empty_dict_without_removals (line 214) is a useful guard for the unchanged {} case.
  • Suggestion: add a multi-key case with threshold_to_diff_deeper=0, e.g. DeepDiff({'a': 1, 'b': 2}, {}, view=COLORED_VIEW, threshold_to_diff_deeper=0). That covers the setting where several removed keys reach this branch.

Suggestions

  • Issue #575 is still open, and its list case is already fixed on dev by #573. Once this lands, the maintainer may be able to close #575.

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