TYP: annotations for internals, set_axis #32376
Conversation
| @@ -561,7 +561,8 @@ def set_axis(self, labels, axis=0, inplace=False): | |||
| obj.set_axis(labels, axis=axis, inplace=True) | |||
| return obj | |||
|
|
|||
| def _set_axis(self, axis, labels) -> None: | |||
| def _set_axis(self, axis: int, labels: Index) -> None: | |||
There was a problem hiding this comment.
labels should be the same as index_like parameter of ensure_index
There was a problem hiding this comment.
thoughts on Best Practices here? I'd like to push up the stack where we do ensure_index. so in this case the annotation is kind of "you should pass Index, but we're not that strict about it"
There was a problem hiding this comment.
it makes sense to limit the types of internal functions to reduce the number of duplicated checks. maybe add a TODO comment here that the ensure_index added is to be eventually removed
There was a problem hiding this comment.
"you should pass Index, but we're not that strict about it"
mypy will be
There was a problem hiding this comment.
mypy will be
im skeptical in this case because it goes through cython. This gets called when you do frame.columns = ["foo"]
There was a problem hiding this comment.
im skeptical in this case because it goes through cython
fair enough, cython is another pain point for us with type annotations.
| @@ -3599,7 +3599,7 @@ def align( | |||
| see_also_sub=" or columns", | |||
| ) | |||
| @Appender(NDFrame.set_axis.__doc__) | |||
| def set_axis(self, labels, axis=0, inplace=False): | |||
| def set_axis(self, labels, axis: Axis = 0, inplace: bool = False): | |||
There was a problem hiding this comment.
return type is easy. for labels I'm ambivalent since it could be listlike, but should be Index
There was a problem hiding this comment.
i take it back, return type depends on inplace
There was a problem hiding this comment.
i take it back, return type depends on inplace
yeah, that is causing us problems without Literal.
| def set_axis(self, axis, new_labels): | ||
| new_labels = ensure_index(new_labels) | ||
| def set_axis(self, axis: int, new_labels: Index): | ||
| # Caller is responsible for ensuring we have an Index object. |
There was a problem hiding this comment.
if this isn't public, the comment isn't needed with type annotations
There was a problem hiding this comment.
is mypy smart enough to enforce this? I'm hoping to ween internals off of ensure_index completely, the comment is to keep track of where it has been done
There was a problem hiding this comment.
my only concern is keeping comments in-sync with code changes over time.
There was a problem hiding this comment.
is mypy smart enough to enforce this?
as we add more type hints throughout the code. (the pace should be quicker but too much bikeshedding IMO) if mypy is green, job done! you can only get so far if the type hints are wrong.
There was a problem hiding this comment.
the pace should be quicker but too much bikeshedding IMO
Fair enough, im happy to defer to you on these
There was a problem hiding this comment.
the pace should be quicker but too much bikeshedding IMO
Fair enough, im happy to defer to you on these
The point is, mypy is the checker, so the reviewer's don't need to be. (there does need to be some consideration of style, readability, maintenance etc)
There was a problem hiding this comment.
(there does need to be some consideration of style, readability, maintenance etc)
and we could start to address those concerns with tools like https://pypi.org/project/flake8-annotations-complexity/
pandas/core/internals/managers.py
Outdated
| @@ -184,7 +184,7 @@ def set_axis(self, axis, new_labels): | |||
|
|
|||
| self.axes[axis] = new_labels | |||
|
|
|||
| def rename_axis(self, mapper, axis, copy: bool = True, level=None): | |||
| def rename_axis(self, mapper, axis: int, copy: bool = True, level=None): | |||
There was a problem hiding this comment.
return type, mapper and level
There was a problem hiding this comment.
looks like level may be inconsistent with the caller's signature, so punting for now. xref #32349.
| @@ -623,7 +623,7 @@ def comp(s, regex=False): | |||
| bm._consolidate_inplace() | |||
| return bm | |||
|
|
|||
| def is_consolidated(self): | |||
| def is_consolidated(self) -> bool: | |||
There was a problem hiding this comment.
as an aside, could this be a property? and remove/inline _consolidate_check
There was a problem hiding this comment.
probably could, would need to do some profiling. xref #32261.
|
Thanks @jbrockmendel |
No description provided.