-
-
Notifications
You must be signed in to change notification settings - Fork 144
type datetimeindex constructor #1120
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
type datetimeindex constructor #1120
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.
caught one issue regarding MultiIndex
pandas-stubs/core/indexes/multi.pyi
Outdated
@@ -44,7 +44,7 @@ class MultiIndex(Index[Any]): | |||
names=..., | |||
dtype=..., | |||
copy=..., | |||
name=..., | |||
name: Hashable = ..., | |||
verify_integrity: bool = ..., | |||
_set_identity: bool = ..., | |||
) -> None: ... |
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.
We have an opportunity for cleanup here. No need to have both __new__()
and __init__()
. We should just have __init__()
. We use __new__()
instead of __init__()
in the stubs when we want to return different subclasses based on the arguments. But for MultiIndex
, that isn't the case. So we can delete __new__()
.
Ideally, we'd have types for all of the arguments, which will have to be based on the 3.0 docs, because the docs did get updated to correspond to what is in the code (which doesn't have _set_identity
.
For this PR, I'm OK if you don't do them all, BUT names
and name
need to be SequenceNotStr[Hashable]
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 Irv! sure, I corrected name
for multiindex, sorry for not having spotted that
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.
Looks like we might need MultiIndex.__new__
too, else the test I added fails pyright with
/home/marcogorelli/pandas-stubs-dev/tests/test_indexes.py:70:62 - error: Argument of type "list[str]" cannot be assigned to parameter "name" of type "Hashable" in function "__new__"
"list[str]" is incompatible with protocol "Hashable"
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.
Interesting. I guess because the parent class uses __new__()
, then the subclass has to do it as well. So for MultiIndex
, we should provide __new__()
and not provide __init__()
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.
The only thing left to do is to remove __init__()
from MultiIndex
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
assert_type()
to assert the type of any return value