CLN: type annotations #29333
CLN: type annotations #29333
Conversation
There was a problem hiding this comment.
Just as a general comment I think it would be better to fully annotate some methods rather than partially annotating a lot of methods. The real value in doing this is getting the hard ones reviewed; can always have newer contributors sprint on easier annotations
pandas/core/groupby/ops.py
Outdated
| @@ -790,7 +790,7 @@ def _get_axes(group): | |||
| return group.axes | |||
|
|
|||
|
|
|||
| def _is_indexed_like(obj, axes): | |||
| def _is_indexed_like(obj, axes) -> bool: | |||
There was a problem hiding this comment.
I think reasonable to annotate obj as FrameOrSeries here, unless the TypeVar doesn't work in which case a Union
That's reasonable. How strong is this preference? i.e. when weighed against "its nice to do these separately from the other PR that makes non-typing changes in the same functions to have a simpler diff" |
I would rather merge quickly on correct changes that are non-controversial, rather block things. so actually happy to type the 'easy' things (of course nice for newcomers to do this too). |
|
FWIW I’m not blocking on this just have reviewed thoroughly. Feel free to merge if happy with it
…Sent from my iPhone
On Nov 2, 2019, at 8:34 AM, Jeff Reback ***@***.***> wrote:
Just as a general comment I think it would be better to fully annotate some methods rather than partially annotating a lot of methods.
That's reasonable. How strong is this preference? i.e. when weighed against "its nice to do these separately from the other PR that makes non-typing changes in the same functions to have a simpler diff"
I would rather merge quickly on correct changes that are non-controversial, rather block things. so actually happy to type the 'easy' things (of course nice for newcomers to do this too).
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
green |
|
gentle ping. a couple of upcoming branches are going to have merge conflicts with this otherwise |
|
thanks |
Separating these out from the non-CLN work going on in/around these functions.