fix: override_if_not_empty should not skip falsy primitives (0, False, 0.0) - #41
Conversation
…, 0.0) The previous implementation used `nxt if nxt else base`, which treats any falsy value — including `0`, `False`, and `0.0` — as "empty" and silently keeps `base`. These are valid non-empty values that should override `base`. Only `None` (null) and sized empty containers (len == 0) qualify as "empty or null" per the strategy's documented contract. Falsy primitives have no `len()` and are neither null nor empty, so they now correctly override `base`.
toumorokoshi
left a comment
There was a problem hiding this comment.
Thanks! this looks right, but a quick comment.
Also although I agree this is a "fix", I think we'll do a major version bump - it's a breaking change to a merge strategy.
|
Parameterized in It asserts the type as well as the value, since Major version bump is your call, that makes sense to me. Also corrected the PR description: it still claimed a |
|
great, thank you! |
Problem
TypeConflictStrategies.strategy_override_if_not_emptyuses a bare truthiness check:This treats any falsy value as "empty", including
0,False, and0.0.These are valid, intentional values — they are not empty, and they are not
null — so they should override
base.Fix
Replace the truthiness check with an explicit null/empty test:
None→ keepbase(null)len(nxt) == 0(dict, list, set, str, …) → keepbase(empty container)0,False,0.0) → returnnxtThe empty check is
isinstance(nxt, Sized) and len(nxt) == 0, so non-sizedtypes (ints, floats, bools) fall through to the override path.
The existing tests pass unchanged; a parameterized test covers the falsy
primitives and the empty/null cases.