-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Adds __hash__
method to @dataclasses.dataclass
, refs #11463
#11496
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
3 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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 the PR! I like that all the possible cases are carefully tested. Left some minor comments, overall looks good.
mypy/plugins/dataclasses.py
Outdated
unsafe_hash = decorator_arguments.get('unsafe_hash', False) | ||
eq = decorator_arguments['eq'] | ||
frozen = decorator_arguments['frozen'] | ||
cond = (unsafe_hash, eq, frozen) |
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 think that this makes the conditions harder to understand. If you think that the conditions would be too verbose otherwise, maybe instead rename unsafe_hash
to something shorter, such as unsafe
. Now you could write something like if not unsafe and eq and not frozen
.
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.
if ((not unsafe_hash and not eq and not frozen)
or (not unsafe_hash and not eq and frozen)):
I think that both are quite hard to read 😞
Mine was inspired by pattern matching technique I use in different functional languages.
Anyways, I've changed it! 👍
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The mypy_primer failure seems to indicate a real issue. For example, this generates a false positive: from dataclasses import dataclass
a = f() # Forward ref to force two semantic analysis passes
@dataclass(unsafe_hash=True)
class C: # Cannot overwrite attribute "__hash__" in class "C"
x: str
def f(): pass It looks like the new logic doesn't work correctly if we need multiple semantic analysis passes. If the previous pass had added a |
Let's try this solution. Now I check that given variable was not plugin generated. |
Can you fix the conflicts? I can do another round of review once the PR is up-to-date. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I somehow nuked my old fork and that's why all my old PRs are now closed. I can recreate some of them. Here's a list of things I still want to get merged:
|
I was a big fan of these, as well :) |
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
Closes #11463
Closes #11495
I still need to add a test case for
3.9
, where there's nounsafe_hash
arg, but__hash__
is still added somehow.Right now I just want to see the CI results.