Skip to content

TYP: Fix typing of frame.explode #43834

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 1, 2021
Merged

TYP: Fix typing of frame.explode #43834

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
74e1e11
TYP: Fix typing of frame.explode
phofl Oct 1, 2021
2e666d2
TYP: Fix typing of frame.explode
phofl Oct 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions pandas/core/frame.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tuple is hashable so can remove that too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm forgot that.Thx

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes of course, if its more strict we should define that. Thanks for the explanation, this helps a lot.

ignore_index: bool = False,
) -> DataFrame:
"""
Expand All @@ -8211,7 +8211,7 @@ def explode(

Parameters
----------
column : Scalar or tuple or list thereof
column : IndexLabel or tuple thereof
Column(s) to explode.
For multiple columns, specify a non-empty list with each element
be str or tuple, and all specified columns their list-like data
Expand Down Expand Up @@ -8293,7 +8293,7 @@ def explode(
if not self.columns.is_unique:
raise ValueError("columns must be unique")

columns: list[Scalar | tuple]
columns: list[Hashable | tuple]
if is_scalar(column) or isinstance(column, tuple):
columns = [column]
elif isinstance(column, list) and all(
Expand Down