TYP: Fix typing of frame.explode #43834
Conversation
phofl
added
Reshaping
pandas/core/frame.py
Outdated
| @@ -8201,7 +8201,7 @@ def stack(self, level: Level = -1, dropna: bool = True): | |||
|
|
|||
| def explode( | |||
| self, | |||
| column: Scalar | tuple | list[Scalar | tuple], | |||
| column: IndexLabel | tuple, | |||
There was a problem hiding this comment.
tuple is hashable so can remove that too.
There was a problem hiding this comment.
@simonjayhawkins got a follow up question: IndexLabel does include Series, DataFrame and arrays? So I think I misunderstood you in the other pr? We should annotate this with Hashable| tuple | list[Hashable| tuple] not IndexLabel, because arrays and so on are not allowed here
There was a problem hiding this comment.
IndexLabel does not yet include Series, DataFrame, Index, EAs etc since they do not satisfy typing.Seqence
For public methods, the function should be as permissible as possible and consistent with other methods accepting similar parameters.
If explode is more restrictive than other methods that accept a columns parameter, we should probably fix. (and maybe should not use the IndexLabel alias yet)
There was a problem hiding this comment.
So we should use IndexLabel always when we have a single column or a list of columns if possible? And when we have only a single column Hashable would be appropriate?
There was a problem hiding this comment.
yes for consistency in public methods. but the types should also reflect the types accepted by the method so where we have discrepancies, mypy should hightlight this as we add more types. Typing of the public api is more difficult, especially since we don't have many of the lower level functions typed.
for internal methods we can be less permissive and say only pass a list around. (The issue then becomes that the list is mutable, so function code could unintentionally change the contents and mypy would catch that.)
There was a problem hiding this comment.
yes of course, if its more strict we should define that. Thanks for the explanation, this helps a lot.
|
@meeseeksdev backport 1.3.x |
|
Something went wrong ... Please have a look at my logs. |
Co-authored-by: Patrick Hoefler <61934744+phofl@users.noreply.github.com>
|
Thanks @phofl for the quick follow-up. backporting typing PRs is not necessary. |
cc @simonjayhawkins