-
-
Notifications
You must be signed in to change notification settings - Fork 143
update signature of Series.pop #627 #631
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
Conversation
pandas-stubs/core/series.pyi
Outdated
@@ -1021,7 +1021,7 @@ class Series(IndexOpsMixin, NDFrame, Generic[S1]): | |||
def droplevel( | |||
self, level: Level | list[Level], axis: AxisIndex = ... | |||
) -> DataFrame: ... | |||
def pop(self, item: _str) -> Series[S1]: ... | |||
def pop(self, item: Hashable) -> Any: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change the result to S1
, not Any
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mypy
is not happy with S1
, it says
error: Return type "S1" of "pop" incompatible with return type "NDFrame" in supertype "NDFrame" [override]
.
Do you know what might be wrong here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know what might be wrong here?
Can you remove pop()
from core/generic.pyi
? That should fix it. I'll review your other changes once you make that change. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I just removed it. Tests pass now (at least locally).
tests/test_series.py
Outdated
) | ||
series = df.loc[0] | ||
res = series.pop(MyEnum.FIRST) | ||
check(assert_type(res, Any), float) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make this test only involve Series
. Something like:
s = pd.Series([3.2, 4.3], index=[MyEnum.FIRST, MyEnum.SECOND], dtype=float)
res = s.pop(MyEnum.FIRST)
check(assert_type(res, float), np.float64)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, it is done in the next commit.
tests/test_series.py
Outdated
check(assert_type(res, float), np.float64) | ||
|
||
s2 = pd.Series([3, 5], index=["alibaba", "zuhuratbaba"], dtype=int) | ||
check(assert_type(s2.pop("alibaba"), int), np.int64) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check(assert_type(s2.pop("alibaba"), int), np.int64) | |
check(assert_type(s2.pop("alibaba"), int), np.int_) |
You'll need to use np.int_
so it passes on Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I didn't know. I just updated it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @anilbey
Updates signature of Series.pop, adds test. The signature follows the corresponding signature in pandas: https://github.com/pandas-dev/pandas/blob/main/pandas/core/series.py#L5014
assert_type()
to assert the type of any return value