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

Simplify and fix overloads for methods with inplace parameter #1105

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

brianhelba
Copy link

The return value of these methods varies on the inplace argument.

Copy link
Collaborator

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

Tests are failing. Please make sure to run the tests locally before resubmitting.

I understand what you are trying to do here, and it seems correct, but I also would like to know what prompted you to make these changes. Is there a test we can add that fails with the current stubs but passes with your changes?

Comment on lines -514 to -520
level: Sequence[Level] = ...,
*,
drop: bool = ...,
name: Level = ...,
inplace: Literal[True],
allow_duplicates: bool = ...,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you can remove this overload.

Copy link
Author

Choose a reason for hiding this comment

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

This seems safe to simplify. There are only 3 return possibilities:

Series or DataFrame or None
When drop is False (the default), a DataFrame is returned. The newly created columns will come first in the DataFrame, followed by the original Series values. When drop is True, a Series is returned. In either case, if inplace=True, no value is returned.

The return value can be fully determined by the value of drop and inplace, otherwise the signatures are identical.


Note, the previous 6 overloads with a split for level: Sequence[Level] and level: Level | None had the same other parameters and possible return values (DataFrame, Series[S1], or None).

I assumed this duplication was a result of some auto-generated code, but if there's something else I'm failing to understand, please let me know.

pandas-stubs/core/series.pyi Outdated Show resolved Hide resolved
The return value of these methods varies on the `inplace` argument.
@brianhelba
Copy link
Author

brianhelba commented Feb 13, 2025

@Dr-Irv Thanks for the quick review and sorry for my delay in replying. I've just updated the PR and tests are now passing.

I understand what you are trying to do here, and it seems correct, but I also would like to know what prompted you to make these changes.

My initial reason for wanting this change was a mistake on my part (I thought that the overloads for Series.rename were not returning the correct type, but it was actually a separate bug in my project 😅). However, after looking at this codebase, I realized that the overloads for inplace seemed needlessly complicated, so I wanted to help clean it up.

There are probably a few minor improvements here as well (adding overloads or return type annotations to methods which lacked them before), but it seemed unnecessary to try and make separate PRs (one for new annotations, one for cleanup).

@brianhelba brianhelba requested a review from Dr-Irv February 13, 2025 20:09
@brianhelba
Copy link
Author

Is there a test we can add that fails with the current stubs but passes with your changes?

We could probably add one for DataFrame.where or DataFrame.mask (places that totally lacked an overload before), but as mentioned, my primary motivation was just cleanup. Removing lines of code is often just as helpful as adding them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants