fix: treat empty property strings as present in helpers - #3801
Open
daviddallakyan2005 wants to merge 3 commits into
Open
fix: treat empty property strings as present in helpers#3801daviddallakyan2005 wants to merge 3 commits into
daviddallakyan2005 wants to merge 3 commits into
Conversation
Walrus `.get` truthiness treated "" as missing. Match the is-not-None check from apache#3660/apache#3745 so a present empty value is parsed (or returned) instead of falling back.
Contributor
|
Thanks for raising this PR @daviddallakyan2005. I think this one is debatable. What's the added value of having an empty string? I think unsetting the property is the right thing to do. |
Contributor
Author
|
That makes sense, thanks. Looking at it again, I agree that an empty string is not useful for typed properties. Unlike I also noticed that I can close this PR, or change the checks to be more explicit without changing the current behavior. Which would you prefer? |
Keep the behavior introduced on main while resolving the property helper conflict.
This was referenced Aug 16, 2026
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3713
Rationale for this change
property_as_int,property_as_float, andget_first_property_valueused a truthy walrus check (if value := properties.get(...)). An empty string is falsy in Python, so a present""was treated as a missing key: the typed helpers returned the default instead of parsing, andget_first_property_valueskipped to the next name.This is the same class of bug as #3660 (falsy
0collapsed to unset) and #3745 (CLI property lookup reported""as missing). These helpers now useis not None, matching those PRs and the typed getters already inConfig.get_int/get_bool. A missing key still returns the default. A present empty string is a value: int and float properties raiseValueError(same as any other unparseable string), andget_first_property_valuereturns"".property_as_boolkeeps the empty-string fallback behavior added onmainin #3492.Other walrus
.getsites (IO, catalog, table locations/metadata) are left for follow-ups.Are these changes tested?
Yes. Regression tests cover empty strings for int and float properties and for
get_first_property_value, while preserving the boolean fallback from #3492.make lintmake test→ 3933 passed, 3 skipped, 1569 deselecteduv run pytest tests/utils/test_properties.py -v→ 14 passed after resolving the conflictAre there any user-facing changes?
Yes. An explicitly empty int or float property now raises
ValueErrorinstead of silently using the default, andget_first_property_valueno longer skips an empty string. Changelog label requested.