TYP: add typing information #8
Conversation
quantity/core.py
Outdated
| try: | ||
| array_api_compat.array_namespace(arg) | ||
| except TypeError: | ||
| return False | ||
| else: |
There was a problem hiding this comment.
Can we turn off the rule that removes this else - it is more readable to keep it!
quantity/_quantity_api.py
Outdated
|
|
||
|
|
||
| @runtime_checkable | ||
| class ArrayQuantity(Quantity, Array, Protocol): |
There was a problem hiding this comment.
QuantityArray may be better?
More relevantly, for my own understanding, an isinstance will check that something has the value and unit atttributes, that those are of the right type, and adds to Quantity that it will also check that the something itself is array-like (i.e., has __array_namespace__), right?
There was a problem hiding this comment.
that those are of the right type
I don't think that's included in "runtime checkable" protocols.
There was a problem hiding this comment.
Yeah, unfortunately not.
There was a problem hiding this comment.
Well, at some level that makes things easy, in that basic Quantity operations will just work with float values.
quantity/core.py
Outdated
|
|
||
|
|
||
| def _make_comp(comp): | ||
| def __comp__(self, other): | ||
| def _make_comp(comp: str) -> Callable[[Quantity, Any], Array]: |
There was a problem hiding this comment.
Should be Union[Array | NotImplementedType], no?
quantity/core.py
Outdated
| def _make_comp(comp): | ||
| def __comp__(self, other): | ||
| def _make_comp(comp: str) -> Callable[[Quantity, Any], Array]: | ||
| def _comp_(self: Quantity, other: Any) -> Array | NotImplementedType: |
There was a problem hiding this comment.
Don't mind changing it at all, but if we do, probably nicer to change __op__ above too.
quantity/core.py
Outdated
| try: | ||
| exp = exp.__complex__() | ||
| out = complex(exp) |
There was a problem hiding this comment.
This is not the same! E.g., complex("1+1j") works, but we don't want to support 2**"1+1j"
quantity/core.py
Outdated
|
|
||
| def __array_namespace__(self, *, api_version: str | None = None) -> Any: | ||
| # TODO: make our own? | ||
| del api_version |
There was a problem hiding this comment.
Why is this needed? Can one not add something like Unused[str | None] ... checking, I see that this was suggested at python/typing#1499 but not liked; instead a relevant noqa to get the linter happy was suggested. That does seem nicer.
quantity/core.py
Outdated
| return np | ||
|
|
||
| def _operate(self, other, op, units_helper): | ||
| def _operate( | ||
| self, other: Any, op: Any, units_helper: Any |
There was a problem hiding this comment.
I think op and units_helper can be
op: str, units_helper: Callable[[Unit, Unit], tuple[list[Callable | None, Callable | None], Unit | None]]
Not sure I did the second one right, but it takes 2 units and returns a list of two converters or None and a final unit or None. Maybe worth defining separately? The general helper could have arbitrary number of units, with the output having the same number of converters.
quantity/core.py
Outdated
| exp = _check_pow_args(exp, mod) | ||
| if exp is NotImplemented: | ||
| def __pow__(self, exp: Any, mod: Any = None) -> Self | NotImplementedType: | ||
| if (mod := _parse_pow_mod(mod)) is NotImplemented: |
There was a problem hiding this comment.
I like the symmetry of the parsing, but maybe we should just go for
if mod is not None:
return NotImplemented
(and then perhaps overload __pow__ itself??)
quantity/core.py
Outdated
| if (mod := _parse_pow_mod(mod)) is NotImplemented: | ||
| return NotImplemented | ||
|
|
||
| if (exp := _check_pow_exp(exp)) is NotImplemented: |
quantity/core.py
Outdated
| @@ -239,17 +294,25 @@ def __pow__(self, exp, mod=None): | |||
| return replace(self, value=value, unit=self.unit**exp) | |||
|
|
|||
| def __ipow__(self, exp, mod=None): | |||
| exp = _check_pow_args(exp, mod) | |||
| if exp is NotImplemented: | |||
| if (mod := _parse_pow_mod(mod)) is NotImplemented: | |||
nstarman
force-pushed
the
typing
branch
2 times, most recently
from
60f06ed to
79d4f22
Compare
| warn_unused_configs = true | ||
|
|
||
| [[tool.mypy.overrides]] | ||
| module = ["quantity._dev.*", "quantity.tests.*"] |
There was a problem hiding this comment.
Just a bit of a flyby - cool to see the mypy setup! But this is out of date - test has moved now (and another reason why that is good!)
|
Agreed! I think this PR will serve only as reference for a few equivalent PRs. |
Signed-off-by: nstarman <[email protected]>
No description provided.