TYP: annotations in indexes #31181
TYP: annotations in indexes #31181
Conversation
|
Hello @jbrockmendel! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2020-01-22 19:29:15 UTC |
| @@ -632,6 +637,10 @@ def shape(self): | |||
| """ | |||
| return self._values.shape | |||
|
|
|||
| def __len__(self) -> int: | |||
There was a problem hiding this comment.
What error was this throwing? Slightly hesitant to do something like this as it could interfere with Mixins that define this otherwise
There was a problem hiding this comment.
pandas/core/base.py:1343: error: Argument 1 to "len" has incompatible type "IndexOpsMixin"; expected "Sized"
There was a problem hiding this comment.
It might make sense to just ignore that line; I think mypy generally doesn't play nice with Mixins so may be a few things like this, but I would imagine the intent is for that to be defined by whatever gets composed with this class
Not a big deal for me either way maybe just a note for other reviewers. Otherwise this lgtm
There was a problem hiding this comment.
@simonjayhawkins thoughts here?
There was a problem hiding this comment.
perhaps for consistency with addition for _values above, __len__ could raise AbstractMethodError instead?
another alternative could be to add a type declaration at the top of the class instead,
__len__: Callable[..., int]
or ignore as @WillAyd suggested.
There was a problem hiding this comment.
@jbrockmendel generally lgtm. some comments but could be follow-on if appropriate.
| @property | ||
| def is_monotonic_increasing(self) -> bool: | ||
| """alias for is_monotonic""" | ||
| # mypy complains if we alias directly |
There was a problem hiding this comment.
probably don't need the comment.
In Index, is_monotonic is an alias for is_monotonic_increasing, whereas here is_monotonic_increasing is an alias for is_monotonic. Would it make sense to make these consistent instead?
Although without going deeper, looks strange that is_monotonic returns Index(self).is_monotonic, maybe the code there, should be here.
There was a problem hiding this comment.
maybe the code there, should be here.
it does seem a little weird, but its because this gets mixed in to Series, so Series.is_monotonic needs to wrap in Index so that it can use IndexEngine
| if self.inferred_type == "string": | ||
| is_justify = False | ||
| elif self.inferred_type == "categorical": | ||
| if is_object_dtype(self.categories): # type: ignore |
There was a problem hiding this comment.
the error here is pandas\core\indexes\base.py:910: error: "Index" has no attribute "categories". should this special case be in CategoricalIndex instead of in the base Index class?
|
thanks! |
No description provided.