-
-
Notifications
You must be signed in to change notification settings - Fork 154
Introduce UnknownSeries and UnknownIndex, type core.strings.pyi using them
#1146
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
core.strings.pyi using them
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 couldn't comment on the code changes suggested below, but I'd like to suggest the following:
- Change the references to
StringMethodsincore/series.pyiandcore/indexes/base.pyito makeSeries[str]andIndex[str]the first argument. Then incore/strings.pyithe first argument of theGenericcalledTwill get bound to that type. - Update the tests for the string methods in
test_series.pyandtest_indexes.pyto test for the return type ofSeries[str]andIndex[str]as appropriate. - For
test_indexes.py, we could use a set of tests on the string methods similar to the ones intest_series.py
If you think we should do this in a separate PR, I'm OK with that as well.
c8e6d8f to
92dc75d
Compare
c7e8187 to
17e280f
Compare
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.
Thanks for doing all of this work. It is a nice improvement to the stubs.
I think that all the methods that have ->T should be -> _TSTR
Because we know these are string methods, so even if the type of the Series (or Index) is unknown, we know we will be returning Series[str] or Index[str]
|
thanks, have updated
I think the only exception is |
|
Regarding #1146 (comment), is it OK if we leave that to a separate PR please? Partially because I feel like the scope here keeps increasing, and partially because I'm not sure it's correct - for example, if I have import pandas as pd
from typing import Any
def func(a: pd.Series[Any]) -> None:
reveal_type(a.str.upper())then with that commit, we get |
OK. It's a |
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.
just a couple of tests to change, otherwise OK
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.
Thanks @MarcoGorelli . Long journey, but a really nice improvement to the stubs!
|
thanks for your careful review, much appreciated! |
One step towards #1133
I think one way to address this issue could be to do it incrementally - when you type a module strictly, add that to the
pyproject.tomlso that it stays strictly typed. Then gradually the partially unknown types will go awayassert_type()to assert the type of any return value🤔 this isn't quite working, trying to fix it up