develop -> infrahub-develop#496
Conversation
Merge stable into develop
WalkthroughRefactors deep_merge_dict in infrahub_sdk/utils.py to use local variables and expand None-handling rules, maintaining dict/list merge behavior. Adds unit tests verifying None-handling per key in tests/unit/sdk/test_utils.py. No public signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Utils as deep_merge_dict
Caller->>Utils: deep_merge_dict(dicta, dictb)
loop for each key in dictb
alt both values are dict
Utils->>Utils: recurse deep_merge_dict(a_val, b_val)
else both values are list
Utils->>Utils: merge lists ([items in A not in B] + B)
else a_val is None and b_val not None
Utils->>Utils: set dicta[key] = b_val
else values equal or b_val is None
Utils->>Utils: no change
else conflict
Utils->>Caller: raise ValueError
end
end
Utils-->>Caller: merged dicta
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## infrahub-develop #496 +/- ##
====================================================
+ Coverage 76.24% 76.25% +0.01%
====================================================
Files 100 100
Lines 9032 9036 +4
Branches 1731 1732 +1
====================================================
+ Hits 6886 6890 +4
Misses 1670 1670
Partials 476 476
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/unit/sdk/test_utils.py (1)
99-104: Optional: expand coverage for edge cases (both None, nested dict with None, and conflicts)Consider adding a few more assertions to lock in behavior and guard regressions:
- Both sides None keeps None
- Nested dict vs None keeps existing nested structure
- Conflicting non-None scalars raise
For example:
def test_deep_merge_dict_none_additional_cases(): # both None -> keep None assert deep_merge_dict({"k": None}, {"k": None}) == {"k": None} # nested dict vs None -> keep existing dict a = {"obj": {"sub": 1}} b = {"obj": None} assert deep_merge_dict(a, b) == {"obj": {"sub": 1}} # conflict on non-None scalar -> raises with pytest.raises(ValueError): deep_merge_dict({"k": "a"}, {"k": "b"})infrahub_sdk/utils.py (4)
148-157: Optional: short-circuit equality and None-on-right before dict/list handlingA small reordering can avoid unnecessary recursion/list-processing when values are equal or when b_val is None. This keeps behavior identical but saves work in common cases.
Apply within this block:
- if isinstance(a_val, dict) and isinstance(b_val, dict): - deep_merge_dict(a_val, b_val, path + [str(key)]) - elif isinstance(a_val, list) and isinstance(b_val, list): - # Merge lists - # Cannot use compare_list because list of dicts won't work (dict not hashable) - dicta[key] = [i for i in a_val if i not in b_val] + b_val - elif a_val is None and b_val is not None: - dicta[key] = b_val - elif a_val == b_val or (a_val is not None and b_val is None): - continue + if a_val == b_val or (a_val is not None and b_val is None): + continue + elif isinstance(a_val, dict) and isinstance(b_val, dict): + deep_merge_dict(a_val, b_val, path + [str(key)]) + elif isinstance(a_val, list) and isinstance(b_val, list): + # Merge lists + # Cannot use compare_list because list of dicts won't work (dict not hashable) + dicta[key] = [i for i in a_val if i not in b_val] + b_val + elif a_val is None and b_val is not None: + dicta[key] = b_val else: raise ValueError("Conflict at %s" % ".".join(path + [str(key)]))
154-157: None-handling semantics look correct; consider documenting explicitlyBehavior now is:
- If a is None and b is not None: take b
- If b is None (and a is not None): keep a
- If equal: keep a
- Otherwise, merge dicts/lists or raise conflict
Recommend updating the function docstring to call out in-place merge semantics and the None rules to prevent misuse.
Here’s suggested docstring text (outside the changed lines):
def deep_merge_dict(dicta: dict, dictb: dict, path: list[str] | None = None) -> dict: """Deep-merge dictb into dicta, modifying dicta in place and returning it. Rules: - Dict vs dict: recursively deep-merge. - List vs list: keep items unique, preserve order: items from dicta not in dictb, then all items from dictb. - None handling (per key): * dicta[key] is None and dictb[key] is not None -> take dictb[key]. * dictb[key] is None (and dicta[key] is not None) -> keep dicta[key]. * equal values -> keep dicta[key]. - Any other non-equal, non-None type/value mismatch -> raise ValueError with a path to the conflict. """
153-153: FYI: list merge is O(n*m); acceptable, but watch for very large listsThe membership check i not in b_val leads to quadratic behavior. If you expect very large lists of hashables, consider a specialized fast-path (e.g., precompute set_b = set(b_val) when all elements are hashable) and fall back to the current approach for unhashables (dicts).
161-161: Nit: reuse b_val alias for consistencydicta[key] = dictb[key] can become dicta[key] = b_val to match the alias pattern introduced above.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
infrahub_sdk/utils.py(1 hunks)tests/unit/sdk/test_utils.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/unit/sdk/test_utils.py (1)
infrahub_sdk/utils.py (1)
deep_merge_dict(138-162)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: unit-tests (3.12)
- GitHub Check: unit-tests (3.13)
- GitHub Check: unit-tests (3.10)
- GitHub Check: unit-tests (3.11)
- GitHub Check: integration-tests-latest-infrahub
- GitHub Check: unit-tests (3.9)
- GitHub Check: unit-tests (3.13)
- GitHub Check: integration-tests-latest-infrahub
🔇 Additional comments (2)
tests/unit/sdk/test_utils.py (1)
99-104: LGTM: Added None-handling test for deep_merge_dict is correct and valuableThis test accurately captures the intended per-key None semantics: None in one dict should not overwrite a non-None in the other, and vice versa.
infrahub_sdk/utils.py (1)
145-147: LGTM: local aliases clarify intent and avoid repeated lookupsUsing a_val and b_val improves readability and slightly reduces repeated indexing. No behavior change.
1 participant
Summary by CodeRabbit
Bug Fixes
Tests