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: Add overload for ser.rename_axis #55412

Merged
merged 2 commits into from
Oct 6, 2023

Conversation

caneff
Copy link
Contributor

@caneff caneff commented Oct 5, 2023

Another overload that improves typing when not using stubs.

@caneff
Copy link
Contributor Author

caneff commented Oct 5, 2023

pre-commit.ci autofix

@caneff caneff changed the title TYP: Add overload for rename_axis TYP: Add overload for ser.rename_axis Oct 5, 2023
@mroeschke mroeschke requested a review from twoertwein October 5, 2023 16:14
@mroeschke mroeschke added the Typing type annotations, mypy/pyright type checking label Oct 5, 2023
pandas/core/series.py Outdated Show resolved Hide resolved
@caneff caneff force-pushed the overload_rename_axis branch from 338a2ad to 697b4e8 Compare October 5, 2023 17:25
@caneff caneff requested a review from twoertwein October 5, 2023 17:25
pandas/core/series.py Outdated Show resolved Hide resolved
@caneff caneff force-pushed the overload_rename_axis branch from 697b4e8 to 482c006 Compare October 5, 2023 18:32
@caneff
Copy link
Contributor Author

caneff commented Oct 5, 2023

pre-commit.ci autofix

@caneff caneff requested a review from twoertwein October 5, 2023 18:45
pandas/core/series.py Outdated Show resolved Hide resolved
@caneff caneff force-pushed the overload_rename_axis branch from 3de1e1c to 540d5e2 Compare October 5, 2023 19:15
Copy link
Member

@twoertwein twoertwein left a comment

Choose a reason for hiding this comment

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

black complains, otherwise, it looks good to me!

@caneff
Copy link
Contributor Author

caneff commented Oct 5, 2023

pre-commit.ci autofix

@@ -5004,6 +5004,42 @@ def reindex( # type: ignore[override]
tolerance=tolerance,
)

@overload
Copy link
Member

Choose a reason for hiding this comment

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

It seems that the error codes are misaligned, see mypy:

pandas/core/series.py:5007: error: Signature of "rename_axis" incompatible with supertype "NDFrame" [override]
[...]
pandas/core/series.py:5008: error: Unused "type: ignore" comment [unused-ignore]
pandas/core/series.py:5020: error: Unused "type: ignore" comment [unused-ignore]
pandas/core/series.py:5032: error: Unused "type: ignore" comment [unused-ignore]
pandas/core/series.py:5044: error: Unused "type: ignore" comment [unused-ignore]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed I think

Copy link
Member

Choose a reason for hiding this comment

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

you might want to run mypy (and black) locally :)

pandas/core/series.py:5007: error: Signature of "rename_axis" incompatible with supertype "NDFrame" [override]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, went and got my precommit setup fixed now and will run locally.

What is the actual issue? Without the ignore it complains about incompatible signatures, and adding the ignore it just says the ignore is unused. So what is the right fix? I see they are incompatible because ser.rename_axis takes less arguments but I don't know what to do about it

Copy link
Member

Choose a reason for hiding this comment

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

Mpyps seems to expect the ignore only on specific lines (5007), not all of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK understood. Now it seems to be not complaining.

@caneff caneff force-pushed the overload_rename_axis branch 2 times, most recently from 0655a12 to ffbc32d Compare October 6, 2023 13:24
@caneff caneff force-pushed the overload_rename_axis branch from ffbc32d to a608be6 Compare October 6, 2023 15:22
@twoertwein twoertwein merged commit 2983464 into pandas-dev:main Oct 6, 2023
33 checks passed
@twoertwein
Copy link
Member

Thank you @caneff !

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.

3 participants