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 cftime.datetime.__richcmp__ #81

Merged
merged 1 commit into from
Nov 12, 2018
Merged

Conversation

bjlittle
Copy link
Collaborator

@bjlittle bjlittle commented Nov 11, 2018

This PR re-addresses the now closed cftime issue #11 (linked to legacy netcdf4-python issue Unidata/netcdf4-python#639) and associated cftime PR #53.

The current implementation of cftime.datetime.__richcmp__ does not align with the old sought after behaviour alluded to in #11, and from my perspective that's a bit of a problem... as it's now not possible to perform (previously supported and operational) comparison operations between cftime.datetime and other custom datetime objects i.e. more specifically an iris PartialDateTime calendar agnostic objects.

For example, it was possible to do the following comparison using a netcdftime.datetime object dt,

pdt1 <= dt < pdt2

where pdt1 and pdt2 are iris PartialDateTime objects.

As it stands at the moment with cftime 1.0.2.1, users are forced to do the equivalent comparison as follows, due to the implementation restrictions of cftime.datetime.__richcmp__,

(pdt1 <= dt) and (pdt2 > dt)

as the current implementation of the cftime.datetime.__richcmp__ method will raise a TypeError if a cftime.datetime object is a LHS operand participating in a rich comparison operation with another custom object (in Python2).

Apologies for the long winded introduction 😰 (I figured that context for this PR would be helpful!)... but this PR bridges the gap back to the old behaviour, and maintains closer consistency with the behaviour between Python2 and Python3 for cftime.datetime.

In a nutshell, for Python3 it's business as usual for cftime.datetime.__richcmp__. However for Python2, rich comparisons with other custom objects will be performed by delegating the comparison to the other object only if it advertises that it supports Cython __richcmp__ or a suitable traditional rich comparison operator i.e. __lt__, __le__, __eq__, __ne__, __gt__, or __ge__, given the context of the current comparison.

A TypeError will only be raised otherwise, thus ensuring that the other object __cmp__ behaviour is circumvented, and more importantly, the default Python object ID comparison is avoided.

For example, if we are doing a Python2 dt < pdt2 comparison, the dt object will first check that the other custom pdt2 object supports either __richcmp__ or __gt__ operations, before delegating the comparison operation to the pdt2 object via returning NotImplemented. Otherwise, a TypeError exception is raised.

Needless to say, the changes in this PR extends cftime.datetime.__richcmp__ support such that it again permits users to do pdt1 <= dt < pdt2`, whoop!

@CLAassistant
Copy link

CLAassistant commented Nov 11, 2018

CLA assistant check
All committers have signed the CLA.

@bjlittle
Copy link
Collaborator Author

bjlittle commented Nov 11, 2018

Heads-up @pdearnshaw ...

Can you confirm that this PR does not undo the use case that was the basis of your originating PR #53?

Thanks

@bjlittle bjlittle changed the title fix cftime.datetime.__richcmp__ Fix cftime.datetime.__richcmp__ Nov 11, 2018
@pdearnshaw
Copy link
Contributor

Just to say that this does not cause any problems with what I was trying to do in #53. In fact it finishes the job in that the python2 case now behaves as I wanted it to but couldn't work out how! I have tested this and it is giving the expected results.

@jswhit jswhit merged commit 5e443ec into Unidata:master Nov 12, 2018
@jswhit
Copy link
Collaborator

jswhit commented Nov 12, 2018

OK then - merging now. Thanks for another useful contribution @bjlittle!

@bjlittle
Copy link
Collaborator Author

bjlittle commented Nov 12, 2018

@jswhit Thanks for responding so swiftly.

When are you aiming to schedule a tag and release? Just curious about your plans...

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