-
-
Notifications
You must be signed in to change notification settings - Fork 123
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
Add missing __hash__ stubs #746
Conversation
@@ -386,6 +386,7 @@ class Timedelta(timedelta): | |||
) -> npt.NDArray[np.bool_]: ... | |||
@overload | |||
def __gt__(self, other: TimedeltaSeries | Series[pd.Timedelta]) -> Series[bool]: ... | |||
def __hash__(self) -> int: ... |
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.
Can you please explain why this is needed: Timedelta inherits from datetime.timedelta
which declares to have __hash__
see typeshed
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.
According to the stubs, Timedelta
does not inherit __hash__
from datetime.timedelta
because Timedelta
defines __eq__
(https://github.com/pandas-dev/pandas-stubs/pull/746/files#diff-0321b1cf2f2c79264daae754a98c26bcac7040336168706d7f12408919c01ad4L313).
Classes that define __eq__
do not inherit __hash__
. Python Docs
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.
okay, I understand it now, if __eq__
is overwritten, __hash__
is set to None
unless it is explicitly declared.
can you please add two quick tests for this?
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.
@twoertwein this is in the pandas repo, so I see no reason not to accept this.
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 for accepting, but typically we like to have tests?
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 for accepting, but typically we like to have tests?
Yes, but in this case, I'm OK to skip the tests.
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.
will merge this pending answer from @twoertwein
These missing
__hash__
methods are in the pandas stubs but are missing here. The missing__hash__
es incorrectly marked these classes as being unhashable.microsoft/pyright#5513 (comment)