fix: preserve falsy values in list_utils helpers - #2120
Conversation
Signed-off-by: Rishi Jat <rishijat098@gmail.com>
|
/cc @degenaro |
There was a problem hiding this comment.
Pull request overview
This PR addresses a bug in trestle/common/list_utils.py where truthiness checks caused valid falsy values (e.g., 0, False, "") to be treated as “unset” and removed from dictionaries; it updates the helpers to use explicit None checks instead.
Changes:
- Update
deep_set()to only pop keys whenvalue is None(unlesspop_if_none=False). - Update
set_or_pop()to set values whenvalue is not Noneand adjust its docstring accordingly.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """Set if value is not None otherwise remove.""" | ||
| if value is not None: | ||
| dic[key] = value | ||
| else: | ||
| dic.pop(key, None) |
There was a problem hiding this comment.
set_or_pop previously removed the key for any falsy value (including empty dict/list), and several current call sites pass dicts that can legitimately be empty (e.g., headers being built up incrementally). With the new value is not None check, an empty dict/list will now be persisted into the header, which can change generated YAML/markdown output (e.g., emitting empty tags) and any logic that treats key presence as meaningful. If the goal is specifically to preserve scalar falsy values (0/False/""), consider popping when value is None OR when value is an empty container (e.g., isinstance(value, (dict, list)) and not value), or add a pop_if_empty parameter and keep existing behavior for current callers.
| """Set if value is not None otherwise remove.""" | |
| if value is not None: | |
| dic[key] = value | |
| else: | |
| dic.pop(key, None) | |
| """Set key for non-None and non-empty container values; otherwise remove key.""" | |
| # Preserve scalar falsy values (0, False, "") but treat empty dicts/lists as absent. | |
| should_pop = value is None or (isinstance(value, (dict, list)) and not value) | |
| if should_pop: | |
| dic.pop(key, None) | |
| else: | |
| dic[key] = value |
| if value is not None or not pop_if_none: | ||
| dic[path[-1]] = value |
There was a problem hiding this comment.
The updated deep_set condition is intended to preserve falsy values, but there isn't test coverage asserting that values like 0, False, or "" are set (and not popped) when pop_if_none=True. Adding regression tests for these cases would prevent reintroducing the truthiness bug.
| def set_or_pop(dic: Dict[str, Any], key: str, value: Any) -> None: | ||
| """Set if value is non-empty list or not None otherwise remove.""" | ||
| if value: | ||
| """Set if value is not None otherwise remove.""" | ||
| if value is not None: | ||
| dic[key] = value | ||
| else: | ||
| dic.pop(key, None) |
There was a problem hiding this comment.
set_or_pop's new semantics (only pop on None) also need regression tests to confirm the desired behavior for falsy scalars (0, False, "") and to lock in the intended behavior for empty containers (e.g., {} / []). Without tests, it's easy for callers that relied on the prior truthiness behavior to regress silently.
|
64cc98c to
4fa5f11
Compare
Issue
Some helpers in list_utils.py use truthiness checks when setting dictionary
values. This causes valid falsy values such as 0, False, or "" to be removed.
Changes
Replace truthiness checks with explicit None checks so falsy values are
preserved while None still removes the key.