fix for boolean type conversion #78
Conversation
There was a problem hiding this comment.
Hi @akurdyukov and thank you for your contribution! Please excuse me for a late reply.
I have added several comments. However, I believe the solution to this should be more fundamental: we should hard-code clickhouse_driver.Client's signature into the plugin's code, so that the code fully controls all of the arguments (in contrast to a pretty faulty **connection_kwargs approach) and converts them to proper types. This will make the behaviour more predictable.
In other words, I believe there should be a preliminary PR which replaces Client(**…) with Client(host=…, …) following its signature from clickhouse-driver here:
And then the PR which you currently work on will easily implement handling this specific str-to-bool case.
| if val in ('y', 'yes', 't', 'true', 'on', '1'): | ||
| return 1 | ||
| elif val in ('n', 'no', 'f', 'false', 'off', '0'): | ||
| return 0 |
There was a problem hiding this comment.
Where do these lists of values come from?
There was a problem hiding this comment.
Just a generic list of possible bool values people use
There was a problem hiding this comment.
Quite biased. Instead let's support only true/false options. It is impossible to guess what a random user may consider to be a true/false str representation.
There was a problem hiding this comment.
Alright, only 'true', 'True', 'fase', 'False' supported
|
Thanks for the review! I fixed most of review comments. Regarding the first one about the method of passing arguments to |
| else: | ||
| raise ValueError("invalid truth value %r" % (val,)) | ||
| raise ValueError(f'invalid truth value {str_value!r}') |
tests/unit/hooks/test_strtobool.py
Outdated
| def test_correct_true(self): | ||
| self.assertTrue(strtobool('true')) | ||
|
|
||
| def test_correct_one(self): | ||
| self.assertTrue(strtobool('1')) | ||
|
|
||
| def test_correct_false(self): | ||
| self.assertFalse(strtobool('false')) | ||
|
|
||
| def test_correct_zero(self): | ||
| self.assertFalse(strtobool('0')) |
There was a problem hiding this comment.
A quick best practice comment: the tests must cover all the supported input values, not only a few. A full coverage is required. However, this is a boilerplate code: you may use self.subTest functionality to check all truthy and all falsy values in a loop instead of creating a test per value.
| else: | ||
| raise ValueError("invalid truth value %r" % (val,)) | ||
| raise ValueError(f'invalid truth value {str_value!r}') |
There was a problem hiding this comment.
Why "truth" value btw? What if it was intended to be a falsy one? :)
| raise ValueError(f'invalid truth value {str_value!r}') | |
| raise ValueError(f'unsupported value: {str_value!r}') |
Hi @akurdyukov, yes, definitely using an external library is an overkill here. I suggest to hardcode all the arguments: |
Update to latest upstream
Fix for #77