Skip to content

fix: correct falsy-value handling in list utility helpers#2127

Open
rishi-jat wants to merge 3 commits intooscal-compass:developfrom
rishi-jat:fix/list-utils-falsy-handling
Open

fix: correct falsy-value handling in list utility helpers#2127
rishi-jat wants to merge 3 commits intooscal-compass:developfrom
rishi-jat:fix/list-utils-falsy-handling

Conversation

@rishi-jat
Copy link
Contributor

@rishi-jat rishi-jat commented Mar 10, 2026

Summary

Fix incorrect handling of falsy values in deep_set and set_or_pop in trestle/common/list_utils.py.

Issue

Both functions used truthiness checks (if value) which caused valid falsy values such as 0, False, empty strings, and empty collections to be treated as missing and removed from dictionaries.

Changes Made

  • Replace truthiness checks with explicit value is not None comparisons.
  • Update logic in:
    • deep_set
    • set_or_pop

Tests

Added tests to ensure falsy values are preserved while None still removes keys.

Signed-off-by: Rishi Jat <rishijat098@gmail.com>
Copilot AI review requested due to automatic review settings March 10, 2026 00:21
@rishi-jat rishi-jat requested a review from a team as a code owner March 10, 2026 00:21
@rishi-jat
Copy link
Contributor Author

/cc @degenaro

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes incorrect handling of falsy values in dictionary helpers so that valid values like 0, False, and '' are not treated as “missing” and removed.

Changes:

  • Update deep_set to use value is not None (instead of truthiness) when deciding whether to set vs. pop.
  • Update set_or_pop to use value is not None (instead of truthiness) when deciding whether to set vs. pop.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 147 to 150
if value is not None or not pop_if_none:
dic[path[-1]] = value
else:
dic.pop(path[-1], None)
Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

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

Tests cover deep_set for None, but not for newly supported falsy values (e.g., 0, False, '') nor for set_or_pop semantics. Adding assertions for these cases would prevent regressions and clarify the intended behavior for falsy values vs. empty containers.

Suggested change
if value is not None or not pop_if_none:
dic[path[-1]] = value
else:
dic.pop(path[-1], None)
if pop_if_none:
set_or_pop(dic, path[-1], value)
else:
dic[path[-1]] = value

Copilot uses AI. Check for mistakes.
Signed-off-by: Rishi Jat <rishijat098@gmail.com>
@rishi-jat rishi-jat force-pushed the fix/list-utils-falsy-handling branch from 7b0abe5 to 641d6cd Compare March 16, 2026 00:20
@rishi-jat
Copy link
Contributor Author

@vikas-agarwal76 please have a look. Thanks!

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.

4 participants