-
-
Notifications
You must be signed in to change notification settings - Fork 153
CLN: remove ignoring bad-param-name-override of pyrefly from pyproject.toml
#1527
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?
CLN: remove ignoring bad-param-name-override of pyrefly from pyproject.toml
#1527
Conversation
| def size(self) -> int: ... | ||
| @overload | ||
| def __getitem__(self, key: ScalarIndexer) -> DTScalarOrNaT: ... | ||
| def __getitem__(self, item: ScalarIndexer) -> DTScalarOrNaT: ... |
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.
Why the change here? I think pandas code has key and not item. Same in a few other places, not sure item is used that much in the pandas code.
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.
Good point! The motivation was to keep pyrefly happy, which requires the param name identical to the one in the base class. But I believe we should really keep them identical to the one in pandas in the concrete implementation, which do not necessarily follow the pyrefly rule.
Incidently, in pandas there are overloads of the concrete implementation which have different param names than the ones in the concrete implementation. I've chosen to follow the concrete implementation.
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.
Basically I have the following example:
from typing import overload
@overload
def foo(x: int) -> int: ...
@overload
def foo(x: str) -> str: ...
def foo(z: int | str):
return z
foo(x=1) # passes type check at this line, but fails at runtimeThis is the implementation in DatetimeLikeArrayMixin.__getitem__ in 2.3.3. in main of pandas they have been made identical.
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.
Good point! The motivation was to keep
pyreflyhappy, which requires the param name identical to the one in the base class. But I believe we should really keep them identical to the one inpandasin the concrete implementation, which do not necessarily follow thepyreflyrule.Incidently, in
pandasthere are overloads of the concrete implementation which have different param names than the ones in the concrete implementation. I've chosen to follow the concrete implementation.
I think the rule of thumb here that is equivalent is that the names we use in the stubs should also correspond to what is in the documentation, which I believe are also the names in the concrete implementation.
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 see the argument name as key in the code:
https://github.com/pandas-dev/pandas/blob/fa1360d41a49173ddfc676c8194ca424871338c8/pandas/core/arrays/datetimelike.py#L376-L385
|
One minor question, rest looks good! |
# pyrefly: ignore[bad-param-name-override]