fix: treat empty property strings as present in helpers - #3801
Open
daviddallakyan2005 wants to merge 1 commit into
Open
fix: treat empty property strings as present in helpers#3801daviddallakyan2005 wants to merge 1 commit into
daviddallakyan2005 wants to merge 1 commit 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.
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
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,property_as_bool, 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). The 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/float/bool raiseValueError(same as any other unparseable string), andget_first_property_valuereturns"".Other walrus
.getsites (IO, catalog, table locations/metadata) are left for follow-ups.Are these changes tested?
Yes. Regression tests pass
""for int, float, and bool properties and forget_first_property_value.make lintmake test→ 3933 passed, 3 skipped, 1569 deselectedAre there any user-facing changes?
Yes. An explicitly empty int/float/bool property now raises
ValueErrorinstead of silently using the default, andget_first_property_valueno longer skips an empty string. Changelog label requested.