Skip to content
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.

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

TYP: sort_index #46300

Merged
merged 12 commits into from
Mar 18, 2022
Merged

TYP: sort_index #46300

merged 12 commits into from
Mar 18, 2022

Conversation

twoertwein
Copy link
Member

Tested the overloads with mypy and pyright:

import pandas as pd

def bool_() -> bool:
    ...

frame = pd.DataFrame()
reveal_type(frame.sort_index()) # DataFrame
reveal_type(frame.sort_index(inplace=True)) # None
reveal_type(frame.sort_index(inplace=False)) # DataFrame
reveal_type(frame.sort_index(inplace=bool_())) # DataFrame | None

series = pd.Series()
reveal_type(series.sort_index()) # Series
reveal_type(series.sort_index(inplace=True)) # None
reveal_type(series.sort_index(inplace=False)) # Series
reveal_type(series.sort_index(inplace=bool_())) # Series | None

@twoertwein twoertwein added the Typing type annotations, mypy/pyright type checking label Mar 10, 2022
@twoertwein twoertwein requested a review from Dr-Irv March 10, 2022 05:39
Copy link
Contributor

@Dr-Irv Dr-Irv left a comment

Choose a reason for hiding this comment

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

make the types consistent (bool vs. bool_t) between generic.py and series.py, frame.py

pandas/core/frame.py Show resolved Hide resolved
@@ -6446,7 +6449,7 @@ def sort_index(

@overload
def sort_index(
self,
self: DataFrameT,
Copy link
Member Author

Choose a reason for hiding this comment

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

as far as I remember, pyright doesn't like self annotations that are broader than the class itself (cannot use NDFrameT).

Copy link
Contributor

@Dr-Irv Dr-Irv left a comment

Choose a reason for hiding this comment

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

Questions about the ascending argument and typing self on all overloads.

pandas/core/frame.py Show resolved Hide resolved
pandas/core/generic.py Show resolved Hide resolved
pandas/core/series.py Show resolved Hide resolved
pandas/core/frame.py Outdated Show resolved Hide resolved
pandas/core/sorting.py Show resolved Hide resolved
pandas/core/sorting.py Show resolved Hide resolved
Copy link
Member

@simonjayhawkins simonjayhawkins left a comment

Choose a reason for hiding this comment

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

Thanks @twoertwein generally lgtm.

pandas/core/series.py Outdated Show resolved Hide resolved
pandas/core/series.py Outdated Show resolved Hide resolved
@simonjayhawkins simonjayhawkins added this to the 1.5 milestone Mar 14, 2022
Copy link
Member

@simonjayhawkins simonjayhawkins left a comment

Choose a reason for hiding this comment

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

Thanks @twoertwein

The requested changes were to keep the scope of this PR narrow and for consistency with the existing code. (and not necessarily the correct thing to do)

#32638 could potentially discuss changes on how the _constructor mechanism works. We maybe need a more limited discussion from a typing perspective considering a trade-off for false positives and more refined return types for the public api.

Also a similar discussion is maybe needed considering a trade-off for false positives and catching more errors using Literal for the Axis parameter in the public api.

I'll leave it to someone else to champion these issues as I try to backseat on the public api typing issues as it generally needs a different approach to the internal typing of the codebase and is therefore difficult to not give out mixed messages on typing policy.

@jreback
Copy link
Contributor

jreback commented Mar 16, 2022

can you merge master

@twoertwein
Copy link
Member Author

green, except for mypy error also present on main

@jreback jreback merged commit 1400649 into pandas-dev:main Mar 18, 2022
@jreback
Copy link
Contributor

jreback commented Mar 18, 2022

thanks @twoertwein (and others!)

@twoertwein twoertwein deleted the sort_index branch April 1, 2022 01:36
yehoshuadimarsky pushed a commit to yehoshuadimarsky/pandas that referenced this pull request Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Typing type annotations, mypy/pyright type checking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants