Skip to content

Make netcdftime.datetime immutable and hashable. Fixes #255. #275

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

Merged
merged 1 commit into from
Jul 8, 2014

Conversation

ddasilva
Copy link
Contributor

@ddasilva ddasilva commented Jul 4, 2014

I went ahead and put together a pull request for the change approved in #255. The ticket approves making netcdftime.datetime immutable so it can be used as a dictionary key. This is my first contribution, so let me of any areas for improvement.

Best,
Daniel

@shoyer
Copy link
Contributor

shoyer commented Jul 4, 2014

Awesome! I'm glad you're able to work on this.

The main complication is that netcdftime.datetime objects can be equal to datetime.datetime objects, so they need to have the same hash value. Otherwise, dictionaries won't work properly.

assert hash(d1) != hash(d3)

# check datetime immutability
try:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cleaner way to write this is:

with self.assertRaises(AttributeError):
    d1.year = 1999

@ddasilva
Copy link
Contributor Author

ddasilva commented Jul 4, 2014

The immutability hack before was to avoid doing a lot repetitive properties, but I agree doing properties is cleaner and more intuitive. I updated the code to use that approach.

Also, the hash function is now coded to use the hash from a datetime.datetime that is equal to it. I added test code to check that a netcdftime.datetime and datetime.datetime that are equal have the same hash value.

@shoyer
Copy link
Contributor

shoyer commented Jul 4, 2014

The last remaining wrinkle would be hashing dates that can't be represented with datetime.datetime -- the entire reason why netcdftime.datetime is necessary (e.g., February 30 with a 360 day calendar). My not so elegant solution would to be use try... except ValueError to catch cases where converting to a real datetime fails.

Finally, this would be good time to bump the version in netcdftime.__version__ (it is versioned separately from the rest of the netCDF4 package, for reasons I'm not entirely sure of).

@ddasilva
Copy link
Contributor Author

ddasilva commented Jul 4, 2014

Sounds reasonable. I implemented the hashing as you suggested for dates that can't be represented with datetime. There is a unit test for that now too. I bumped the version up one minor number from 1.1 to 1.2.

@shoyer
Copy link
Contributor

shoyer commented Jul 6, 2014

Looks good to go to me! 👍

@ddasilva
Copy link
Contributor Author

ddasilva commented Jul 6, 2014

Great! Let me know of anything else that needs to be done to merge.

@shoyer
Copy link
Contributor

shoyer commented Jul 8, 2014

It's up to @jswhit now.

jswhit added a commit that referenced this pull request Jul 8, 2014
Make netcdftime.datetime immutable and hashable. Fixes #255.
@jswhit jswhit merged commit 98f35b1 into Unidata:master Jul 8, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants