Skip to content
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

Fix rich comparisons for datetime objects #53

Merged
merged 3 commits into from
Aug 20, 2018

Conversation

pdearnshaw
Copy link
Contributor

@pdearnshaw pdearnshaw commented Jul 9, 2018

As identified in #11, rich comparisons for cftime.datetime objects with other unknown objects do not work as cftime raises a TypeError instead of returning NotImplemented. This pull request is to fix this.

While this works in Python 3, it should be noted that Python 2 is more problematic. The unit test not_comparable_4 in test_richcmp will fail as cftime.datetime objects are treated as comparable with integer zero and so the TypeError is not raised when it is expected.

@CLAassistant
Copy link

CLAassistant commented Jul 9, 2018

CLA assistant check
All committers have signed the CLA.

@jswhit
Copy link
Collaborator

jswhit commented Jul 20, 2018

The python 2.7 Travis tests are failing. We can't merge until this is fixed.

@pdearnshaw
Copy link
Contributor Author

This was alluded to in the description and I am still trying to work out the best way of dealing with it. At present the only way I have is to add

if sys.version_info[0] < 3:
    raise TypeError("cannot compare {0!r} and {1!r}".format(self, other))
else:
    return NotImplemented

This solves the original problem for Python 3 and allows the code to behave as before for Python 2, but in Python 2 code the original problem will still exist. As far as I can tell there is no way in Python 2 to separate objects that should be comparable to cftime.datatime objects and those that shouldn't be without being explicit (e.g. the way datetime objects are handled), or prescribing a structure on those objects that want to be comparable.

Would this be acceptable?

Paul Earnshaw added 2 commits July 24, 2018 12:10
@jswhit
Copy link
Collaborator

jswhit commented Jul 27, 2018

I'm OK with this. @jhamman?

@jhamman
Copy link
Collaborator

jhamman commented Jul 27, 2018

I'm also fine with this but would appreciate a test to validate the expected behavior.

@jswhit
Copy link
Collaborator

jswhit commented Aug 15, 2018

I can merge this as soon as a test is added

@jswhit
Copy link
Collaborator

jswhit commented Aug 20, 2018

I'm going to go ahead and merge this, without a test for now.

@jswhit jswhit merged commit 88d3ca8 into Unidata:master Aug 20, 2018
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.

4 participants