Make sure base_value is always float
#874
Conversation
github-actions
bot
added
part:docs
Mainly for DRY, but also to have a central point to fix bugs, like `base_value` not being always `float`. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
llucax
force-pushed
the
fix-quantity-zero
branch
from
37ae784 to
8a045c2
Compare
In Python `float` is a bit weird, because `isinstance(1, float)` is `False` but still `int` is a subtype of `float` in terms of the type system, so `int` will be accepted when a `float` is specified in arguments for example. This leads to quantities that can have `int` as base value if it was constructed with a `int` instead of a `float`, which is unintuitive when the `base_value` type hint is `float` and can potentially cause problems. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
llucax
force-pushed
the
fix-quantity-zero
branch
from
8a045c2 to
674a534
Compare
|
Updated to address the comments. |
llucax
force-pushed
the
fix-quantity-zero
branch
from
b2459ce to
563c63b
Compare
|
Haha, you can clearly see I wrote that in a rush :P Updated. |
|
Enabled auto-merge. |
llucax
force-pushed
the
fix-quantity-zero
branch
from
563c63b to
af027ef
Compare
This comment was marked as outdated.
This comment was marked as outdated.
llucax
force-pushed
the
fix-quantity-zero
branch
2 times, most recently
from
8779dec to
af027ef
Compare
This comment was marked as outdated.
This comment was marked as outdated.
`test_to_and_from_string()` is **extremely** fragile, even the most innocent change can break it, so we add some extra comments to make sure other people don't attempt to change it and avoid them some headaches. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com> Co-authored-by: daniel-zullo-frequenz <120166726+daniel-zullo-frequenz@users.noreply.github.com>
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
llucax
force-pushed
the
fix-quantity-zero
branch
from
af027ef to
15c0d14
Compare
This comment was marked as outdated.
This comment was marked as outdated.
| # an empty list. With this we should at least make sure we are not testing less classes | ||
| # than before. We don't get the actual number using len(_QUANTITY_SUBCLASSES) because it | ||
| # would defeat the purpose of the test. | ||
| _SANITFY_NUM_CLASSES = 7 |
There was a problem hiding this comment.
A bit late to the review, but it might be better to call this _SANITIFY_NUM_CLASSES.. I just don't see a reason to skip this extra I...
There was a problem hiding this comment.
just try to pronounce it with and without :P
There was a problem hiding this comment.
I agree completely. When making typos, please make sure you don't skimp on the vowels. :D
There was a problem hiding this comment.
I have no idea how I ended up typing this. But luckily we have smart completion, so typos are properly propagated and the code never fails!
| ) | ||
| ] | ||
|
|
||
| # A very basic sanity check that are messing up the introspection |
There was a problem hiding this comment.
# A very basic sanity check that are messing up the introspection
Someone def. messed up this comment :)
| for unit in _QUANTITY_BASE_UNIT_STRINGS: | ||
| assert unit is not None | ||
|
|
||
| _QUANTITY_CTORS = [ |
There was a problem hiding this comment.
So close, we could have called these QTY_CTORS and pronounced them the "cutty cutters"
There was a problem hiding this comment.
I think I made my first meme-PR!
| # an empty list. With this we should at least make sure we are not testing less classes | ||
| # than before. We don't get the actual number using len(_QUANTITY_SUBCLASSES) because it | ||
| # would defeat the purpose of the test. | ||
| _SANITFY_NUM_CLASSES = 7 |
There was a problem hiding this comment.
I agree completely. When making typos, please make sure you don't skimp on the vowels. :D
| # ) | ||
| # But we can't do that now, because, you guessed it, it will also break the tests | ||
| # (_new() will use 10.0**exponent instead of 10**exponent, which seems to have some | ||
| # effect on the tests. |
There was a problem hiding this comment.
By "on the tests", I guess you mean just this test, because hypothesis also generates some int inputs when we give it a float range?
This function seems to just compare the value from quantity_type.from_string with the value from quantity._base_value. If they don't match up, maybe the from_string method just needs to convert to float as well?
And then they can be compared with quantity.is_close rather than a string comparison?
In Python
floatis a bit weird, becauseisinstance(1, float)isFalsebut stillintis a subtype offloatin terms of the type system, sointwill be accepted when afloatis specified in arguments for example.This leads to quantities that can have
intas base value if it was constructed with aintinstead of afloat, which is unintuitive when thebase_valuetype hint isfloatand can potentially cause problems, in particular when addressing #821.