-
Notifications
You must be signed in to change notification settings - Fork 523
Make classes hashable #389
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
|
👍 |
|
Hmm... Would this be considered a break in backwards compatibility, or no? I don't know of any situations where you would count on something not being hashable, so I'm inclined to say that this does not break backwards compat, but I'm curious to know if anyone can think of a counter-example where the change in this behavior might be a problem. |
|
Actually, looking back on #296, I see that I explicitly decided that this behavior was to be considered "undefined" at the time of merging:
I think it's reasonable to say that in the unlikely event that anyone was counting on this specific behavior, they shouldn't have been. We can merge this as part of |
| else: | ||
| return "%s(%+d)" % (s, self.n) | ||
|
|
||
| # vim:ts=4:sw=4:et |
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.
In the future I think maybe don't add this comment at the bottom. It's already there in a few other files, so it's not a big deal, but I think editor-specific detritus probably doesn't belong in upstream repos.
This builds on #296 by making those classes hashable by explicitly providing the
__hash__methodclasses touched: