-
-
Notifications
You must be signed in to change notification settings - Fork 150
fix(series): #1372 🧱✖️ cumprod #1374
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
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.
One small naming issue in the test, otherwise good to go
@overload | ||
def cumprod( | ||
self, | ||
self: SupportsGetItem[Scalar, _SupportsMul[S1]], |
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.
Does that work with classes that have overloads for __mul__
? My understanding from microsoft/pyright#6549 (comment) is that type checkers will then pick the first overload. It might be okay in this case (and I wish we could use this pattern!) - just something to be aware of when using this patterns in more places.
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.
Not sure that it would matter, because S1
are types we define in pandas
as well as fundamental types (int
, float
, etc.), so I think we will be fine. It's a bit different than the case you wrote up, because of the use of _SupportsMul[S1]
requiring the type that supports __mul__()
to be in S1
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 with this pattern can help us saving efforts, too.
- Note that there are already
SupportsAdd
andSupportsMul
in_typeshed
. They are however different somehow from our_SupportsAdd
and_SupportsMul
. We probably need a renaming in the near future. - It would be great if we could work out a pattern that deal with typing changes like
A.__operate__(B) ->B
andA.__operate__(A) -> B
, for examplebool + int -> int
,int / int -> 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.
Thanks @cmp0xff
assert_type()
to assert the type of any return valueThank you @hamdanal, the idea in #1362 really helps. Also grateful to @Dr-Irv for tagging me there.