Allow multiplying Quantity by float
#875
Conversation
github-actions
bot
added
part:docs
llucax
force-pushed
the
quantity-mul-float
branch
from
1602271 to
66375c2
Compare
This is only applying a factor so it should be safe for any Quantity. To be able to use nicer names in overloads, we use positional-only arguments for `__mul__()`, which in practice shouldn't be much of a breaking change as these methods should be used mostly via operators. We also use `match` syntax in the changed methods. Signed-off-by: Leandro Lucarella <[email protected]>
These methods have a generic `type: ignore` with no clarification why. The issue is we need the ignore here because otherwise `mypy` will give this error: > Overloaded operator methods can't have wider argument types in > overrides The problem seems to be when the other type implements an **incompatible** __rmul__ method, which is not the case here, so we should be safe. Please see this example: https://github.com/python/mypy/blob/c26f1297d4f19d2d1124a30efc97caebb8c28616/test-data/unit/check-overloading.test#L4738C1-L4769C55 And a discussion in a mypy issue here: python/mypy#4985 (comment) For more information. This commit uses a more specific `type: ignore[override]` and add a comment clarifying this. Signed-off-by: Leandro Lucarella <[email protected]>
We make the tests more generic, testing with all constructors, and use hypothesis to have a better (and more random) coverage. Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
llucax
force-pushed
the
quantity-mul-float
branch
from
66375c2 to
5118985
Compare
|
Reabsed on top of the merged #874, this is ready for review @daniel-zullo-frequenz / |
Co-authored-by: daniel-zullo-frequenz <[email protected]> Signed-off-by: Leandro Lucarella <[email protected]>
|
Thanks @daniel-zullo-frequenz, I applied all your suggestions. Since it affects different commits, I'd rather leave the applied suggestions as a separate commit to avoid too much rabasing and amending. |
| # https://github.com/python/mypy/blob/c26f1297d4f19d2d1124a30efc97caebb8c28616/test-data/unit/check-overloading.test#L4738C1-L4769C55 | ||
| # And a discussion in a mypy issue here: | ||
| # https://github.com/python/mypy/issues/4985#issuecomment-389692396 | ||
| @overload # type: ignore[override] |
There was a problem hiding this comment.
Just to add some more context, when Marenz was adding these, we had no idea what the issue was, because without these, mypy kept crashing. Looks like they've fixed that bug and now we are able to understand the issue.
There was a problem hiding this comment.
Yeah, I didn't get a crash (thankfully!) but I still had to do my research, because the error message is not very clear why is there a problem with doing that.
|
|
||
| quantity *= percentage | ||
| print(f"*{quantity=}") | ||
| assert quantity.base_value == expected_value |
There was a problem hiding this comment.
do we need to retain the print statements? don't they pollute the test output when trying to debug something else?
There was a problem hiding this comment.
They are useful when something fails. I guess if you are focusing on some other test you can always run pytest test_other.py::my_interesting_test if you want to keep the noise down. If someone wants to remove it in the future, I'm also fine with that, as if one gets a failure is not that hard to add the print() (except maybe if it is only happening in the CI).
There was a problem hiding this comment.
Well there's always:
assert quantity.base_value == expected_value, f"{quantity=} * {percentage=} != {expected_value}! 🙀 "3 participants
This is only applying a factor so it should be safe for any Quantity.
This PR also improve multiplication tests to use hypothesis and test with all constructors.