Use 'strtobool' instead of comparing with a string.#988
Conversation
|
Thank you for cleaning this up @ndrluis I think it would also make sense to move |
|
Yes, I think that makes sense. What do you think about extracting the methods from the |
|
Yes that sounds like a good idea - the name does sound a bit redundant leaving it as it is:
|
| value_upper = self.value.upper() | ||
| if value_upper in ["TRUE", "FALSE"]: | ||
| return BooleanLiteral(value_upper == "TRUE") | ||
| return BooleanLiteral(strtobool(value_upper)) |
There was a problem hiding this comment.
I feel as though this is an over-optimization, since we check whether value_upper is in "TRUE" and "FALSE". WDYT?
An alternative is to instead just use strtobool for the whole function, but that would mean that we support a string expression like:
"col_A == 'off'" where col_A is a boolean column and 'off' gets parsed as False, and that feels unnecessarily over permissive. Should we leave this block of code as it is?
There was a problem hiding this comment.
Yeah, in this case, it makes sense to be strict.
There was a problem hiding this comment.
I reverted and added more tests to clarify that it needs to be more strict.
There was a problem hiding this comment.
Thank you! It looks like there's still one breaking test with the new changes:
FAILED tests/expressions/test_literals.py::test_string_to_boolean_literal - AssertionError: assert BooleanLiteral(False) == BooleanLiteral(True)
+ where BooleanLiteral(False) = <function StringLiteral.to at 0x7eff9029e4d0>(BooleanType())
+ where <function StringLiteral.to at 0x7eff9029e4d0> = literal('FALSE').to
+ where literal('FALSE') = literal('FALSE')
+ and BooleanType() = BooleanType()
+ and BooleanLiteral(True) = literal(True)
There was a problem hiding this comment.
Oh, my bad. I copied and pasted it but forgot to rerun the test.
| default: bool, | ||
| ) -> bool: | ||
| if value := properties.get(property_name): | ||
| return strtobool(value) |
There was a problem hiding this comment.
nit: perhaps catch and rethrow ValueError to match the other functions
There was a problem hiding this comment.
Adjusted in dd82e20 and I added some missing sad path tests.
sungwy
left a comment
sungwy
left a comment
There was a problem hiding this comment.
LGTM @ndrluis thank you for cleaning this up and adopting all the nitty feedback. Looking forward to using these cleaned up functions in my PRs as well.
@kevinjqliu thank you for your thorough review!
|
Thanks for the contribution @ndrluis! |
* Use 'strtobool' instead of comparing with a string. * Move the PropertyUtil methods to the properties module as functions * fixup! Use 'strtobool' instead of comparing with a string. * fixup! Use 'strtobool' instead of comparing with a string.
* Use 'strtobool' instead of comparing with a string. * Move the PropertyUtil methods to the properties module as functions * fixup! Use 'strtobool' instead of comparing with a string. * fixup! Use 'strtobool' instead of comparing with a string.
3 participants
No description provided.