TYP: Arraylike #31574
TYP: Arraylike #31574
Conversation
|
makes sense from the part i understand (i.e. not Protocol) Does it matter that MultiIndex doesnt have |
| @@ -584,7 +585,7 @@ def _shallow_copy(self, obj, **kwargs): | |||
| return self._constructor(obj, **kwargs) | |||
|
|
|||
|
|
|||
| class IndexOpsMixin: | |||
| class IndexOpsMixin(Generic[ArrayLike]): | |||
There was a problem hiding this comment.
Hmm I wouldn't really consider a Mixin to be a Generic class - have you seen this pattern used elsewhere?
There was a problem hiding this comment.
Hmm I wouldn't really consider a Mixin to be a Generic class
see errors in OP of #31518
pandas/core/indexes/base.py:246: error: Type variable "pandas._typing.ArrayLike" is unbound
pandas/core/indexes/base.py:246: note: (Hint: Use "Generic[ArrayLike]" or "Protocol[ArrayLike]" base class to bind "ArrayLike" inside a class)
pandas/core/indexes/base.py:246: note: (Hint: Use "ArrayLike" in function signature to bind "ArrayLike" inside a function)
The goal here is to use the ArrayLike alias from pandas._typing to replace uses of Union[ExtensionArray, np.ndarray]. ArrayLike is a typevar and hence mypy reports errors if the typevar is not bound.
In Index we want the return type of _values to be the same type as self._data. The way to bind typevars within a class is to use Generic.
More value would come from having the correct type available in calling code instead of a Union type to reduce casts, asserts and type ignores to perform narrowing.
I'll mark this as draft till this is needed.
There was a problem hiding this comment.
Thanks. So my question was more around the general approach of dealing with Mixins here. The mypy suggested approach I think is using Protocol:
https://mypy.readthedocs.io/en/stable/more_types.html#mixin-classes
So just curious if you found this approach elsewhere in the docs or are using another project as a precedent
There was a problem hiding this comment.
The mypy suggested approach I think is using Protocol:
https://mypy.readthedocs.io/en/stable/more_types.html#mixin-classes
from those docs
one can define a protocol that defines common functionality for host classes instead of adding required abstract methods to every mixin
At the moment _values has already been added (not in this PR) as an abstract method on L680 so changing to Protocol is orthogonal to this PR.
PEP 544 states
Programmers are free to not use them even if they use type annotations.
There is no intent to make protocols non-optional in the future.
we can have the discussion regarding the use of protocol and the migration path in #33274
This PR is about binding Typvars.
In common with the Generic abc, Protocol is generic and would still need the typevar in the (Protocol) definition class...
(example from PEP 544)
class Box(Protocol[T_co]):
def content(self) -> T_co:
...
so if we adopt Protocols in the future we would use SomeClassWithJustTheAbstractMethods[Protocol[ArrayLike]] instead of IndexOpsMixin(Generic[ArrayLike]) with the abstract methods included.
So my question was more around the general approach of dealing with Mixins here
hopefully answered as not relevant.
So just curious if you found this approach elsewhere in the docs or are using another project as a precedent
following advice from mypy pandas/core/indexes/base.py:246: note: (Hint: Use "Generic[ArrayLike]" or "Protocol[ArrayLike]" base class to bind "ArrayLike" inside a class)
something we need to watch out for. |
|
There are many occurrences of isinstance(..., MultiIndex) that would presumably be unnecessary if MutliIndex obeyed Liskov but that's another issue. |
|
|
||
| def map_f(values, f): | ||
| return values.map(f) | ||
|
|
There was a problem hiding this comment.
this is changed to silence pandas/core/base.py:1151: note: Internal mypy error checking function redefinition
| @@ -321,7 +321,9 @@ def test_constructor_mixed(self): | |||
| def test_constructor_simple_new(self): | |||
| idx = period_range("2007-01", name="p", periods=2, freq="M") | |||
|
|
|||
| with pytest.raises(AssertionError, match="<class .*PeriodIndex'>"): | |||
| with pytest.raises( | |||
| TypeError, match="_simple_new expects PeriodArray, got PeriodIndex" | |||
There was a problem hiding this comment.
nitpick: i prefer msg = "_simple_new ..." on the previous line so the with pytest.raises ` can be a 1-liner. not a big deal
pandas/core/indexes/period.py
Outdated
| if not isinstance(values, PeriodArray): | ||
| raise TypeError( | ||
| f"_simple_new expects PeriodArray, got {type(values).__name__}" | ||
| ) |
There was a problem hiding this comment.
I think an assertion is correct here since this is internal and annotated
There was a problem hiding this comment.
I think an assertion is correct here since this is internal and annotated
we have tests to test these.
|
At least some of the test failures look related to the exception mesages |
don't really want to be changing these. from https://docs.python.org/3/library/typing.html
when using Generic, we get test failures on the output of The test failures are only 3.6, so assume it is due to the metaclass. from https://www.python.org/dev/peps/pep-0484/#instantiating-generic-classes-and-type-erasure
the errors we are getting are presumably from the way assert gets the repr from a class attribute in python 3.6 |
reopening #31518
cc @jbrockmendel @WillAyd