-
-
Notifications
You must be signed in to change notification settings - Fork 156
GH1541 Revert Series[Any].__add__(str) #1542
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
base: main
Are you sure you want to change the base?
GH1541 Revert Series[Any].__add__(str) #1542
Conversation
Dr-Irv
left a comment
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.
I'm fine with this. Would like @cmp0xff to take a look as well.
cmp0xff
left a comment
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.
I think we still miss a few tests.
I also plan to simplify the tests structure for arithmetic operations, removing the arithmetic subfolder and move everything one level above. Hopefully this will confuse people less in the future.
| def pivot_table( | ||
| data: DataFrame, | ||
| values: _PivotTableValuesTypes[ | ||
| values: _PivotTableValuesTypes[ # ty: ignore[non-subscriptable] |
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.
It's a bit surprising that we now need to ty: ignore's here. Would be great if you could report it to ty.
| def __gt__(self, other: Self | S1) -> np_1darray_bool: ... # type: ignore[override] # pyright: ignore[reportIncompatibleMethodOverride] | ||
| @overload | ||
| def __add__(self: Index[Never], other: _str) -> Never: ... | ||
| def __add__(self: Index[Never], other: _str) -> Index[_str]: ... |
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.
Please add tests in tests/indexes/arithmetic/test_add.py
| sr = df[0] | ||
|
|
||
| check(assert_type(sr + "c1", "pd.Series[str]"), pd.Series, str) | ||
| check(assert_type("c1" + sr, "pd.Series[str]"), pd.Series, str) |
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.
series/arithmetic/str/test_add.pyis meant to test for knownSeries[str]. ForSeries[Any]at type checking, the plan is to useseries/arithmetic/test_add.py. Please move this part there.- We also have a
left_str = pd.DataFrame({"a": ["1", "2", "3_"]})["a"]defined in that file.
- We also have a
- Please also make the undunder methods
addandraddwork
| from typing import ( | ||
| TypeAlias, | ||
| ) |
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.
| from typing import ( | |
| TypeAlias, | |
| ) | |
| from typing import TypeAlias |
| from typing_extensions import ( | ||
| Never, | ||
| assert_type, | ||
| ) |
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.
| from typing_extensions import ( | |
| Never, | |
| assert_type, | |
| ) | |
| from typing_extensions import assert_type |
| if TYPE_CHECKING_INVALID_USAGE: | ||
| assert_type(left_i + s, Never) | ||
| assert_type(s + left_i, Never) | ||
| # relaxing typing, won't work at runtime though |
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.
| # relaxing typing, won't work at runtime though | |
| # GH1541 relaxing typing, won't work at runtime though |
assert_type()to assert the type of any return value)AGENTS.md.