Skip to content

Conversation

@mrigor
Copy link
Contributor

@mrigor mrigor commented Jun 2, 2017

This builds on #296 by making those classes hashable by explicitly providing the __hash__ method

classes touched:

  • relativedelta
  • weekday

@ngould
Copy link

ngould commented Jun 2, 2017

👍

@pganssle
Copy link
Member

pganssle commented Jun 3, 2017

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.

@ngould
Copy link

ngould commented Jun 3, 2017

@pganssle relativedelta's were hashable as of dateutil 2.2. At some point between 2.2 and 2.6 they stopped being hashable, seemingly because of efforts at python 3 compatibility (see #296).

So the backwards compatibility break already occurred. This PR would fix it going forward.

@pganssle
Copy link
Member

pganssle commented Jun 3, 2017

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 would normally say we should add some tests, but I don't know that we want to signal that this behavior is definitely permanent. I could see the some possibility for adding a hash function in to the time zone objects, at least, at some point in the future, so I'm just going to merge as is.

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 2.7.0. I'm going to do a bit of work today on the time zones then fork off a 2.6.x branch to wrap up bugfixes to release before 2.7.0, then I can start merging some of these features into master. Perhaps for a 2.7.0 release we should finalize the hashing behavior of all objects.

@rowillia
Copy link
Contributor

rowillia commented Jun 7, 2017

@pganssle @ngould Right, before #296 the hash behavior was undefined. In Python 2 the hash and eq implementations disagreed (hash would default to object id). In Python 3 these objects weren't hashable. That PR simply made the behavior consistent between 2 and 3.

else:
return "%s(%+d)" % (s, self.n)

# vim:ts=4:sw=4:et
Copy link
Member

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.

@pganssle pganssle merged commit f04798c into dateutil:master Jul 16, 2017
@pganssle pganssle mentioned this pull request Mar 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants