Merge pull request #81 from bjlittle/fix-richcmp

Fix cftime.datetime.__richcmp__

Author: Jeff Whitaker

cftime/_cftime.pyx

@@ -2,7 +2,8 @@
 Performs conversions of netCDF time coordinate data to/from datetime objects.
 """
 
-from cpython.object cimport PyObject_RichCompare
+from cpython.object cimport (PyObject_RichCompare, Py_LT, Py_LE, Py_EQ,
+                             Py_NE, Py_GT, Py_GE)
 import cython
 import numpy as np
 import re
@@ -37,6 +38,9 @@ cdef int[12] _dpm_360  = [30, 30, 30, 30, 30, 30, 30, 30, 30, 30, 30, 30]
 # Same as above, but SUM of previous months (no leap years).
 cdef int[13] _spm_365day = [0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334, 365]
 cdef int[13] _spm_366day = [0, 31, 60, 91, 121, 152, 182, 213, 244, 274, 305, 335, 366]
+# Reverse operator lookup for datetime.__richcmp__
+_rop_lookup = {Py_LT: '__gt__', Py_LE: '__ge__', Py_EQ: '__eq__',
+               Py_GT: '__lt__', Py_GE: '__le__', Py_NE: '__ne__'}
 
 __version__ = '1.0.2.1'
 
@@ -1272,16 +1276,29 @@ Gregorial calendar.
                 raise TypeError("cannot compare {0!r} and {1!r} (different calendars)".format(self, other))
             return PyObject_RichCompare(dt.to_tuple(), to_tuple(other), op)
         else:
-            # With Python3 we can use "return NotImplemented". If the other
-            # object does not have rich comparison instructions for cftime
-            # then a TypeError is automatically raised. With Python2 in this
-            # scenario the default behaviour is to compare the object ids
-            # which will always have a result. Therefore there is no way to
-            # differentiate between objects that do or do not have legitimate
-            # comparisons, and so we cannot remove the TypeError below.
-            if sys.version_info[0] < 3:
-                raise TypeError("cannot compare {0!r} and {1!r}".format(self, other))
+            # With Python3 we can simply return NotImplemented. If the other
+            # object does not support rich comparison for cftime then a
+            # TypeError will be automatically raised. However, Python2 is not
+            # consistent with this Python3 behaviour. In Python2, we only
+            # delegate the comparison operation to the other object iff it has
+            # suitable rich comparison support available. This is deduced by
+            # introspection of the other object. Otherwise, we explicitly raise
+            # a TypeError to avoid Python2 defaulting to using either __cmp__
+            # comparision on the other object, or worst still, object ID
+            # comparison. Either way, at this point the comparision is deemed
+            # not valid from our perspective.
+            if sys.version_info.major == 2:
+                rop = _rop_lookup[op]
+                if (hasattr(other, '__richcmp__') or hasattr(other, rop)):
+                    # The other object potentially has the smarts to handle
+                    # the comparision, so allow the Python machinery to hand
+                    # the operation off to the other object.
+                    return NotImplemented
+                # Otherwise, the comparison is not valid.
+                emsg = "cannot compare {0!r} and {1!r}"
+                raise TypeError(emsg.format(self, other))
             else:
+                # Delegate responsibility of comparison to the other object.
                 return NotImplemented
 
     cdef _getstate(self):

test/test_cftime.py

@@ -1,5 +1,8 @@
 from __future__ import print_function
+
 import copy
+import operator
+import sys
 import unittest
 from collections import namedtuple
 from datetime import datetime, timedelta
@@ -8,13 +11,13 @@
 import pytest
 from numpy.testing import assert_almost_equal, assert_equal
 
+import cftime
 from cftime import datetime as datetimex
 from cftime import real_datetime
 from cftime import (DateFromJulianDay, Datetime360Day, DatetimeAllLeap,
                     DatetimeGregorian, DatetimeJulian, DatetimeNoLeap,
                     DatetimeProlepticGregorian, JulianDayFromDate, _parse_date,
                     date2index, date2num, num2date, utime)
-import cftime
 
 # test cftime module for netCDF time <--> python datetime conversions.
 
@@ -1106,12 +1109,75 @@ def not_comparable_3():
             self.datetime_date1 > self.date2_365_day
 
         def not_comparable_4():
-            "compare a datetime instance to something other than a datetime"
+            "compare a datetime instance to non-datetime"
             self.date1_365_day > 0
 
+        def not_comparable_5():
+            "compare non-datetime to a datetime instance"
+            0 < self.date_1_365_day
+
         for func in [not_comparable_1, not_comparable_2, not_comparable_3, not_comparable_4]:
             self.assertRaises(TypeError, func)
 
+    @pytest.mark.skipif(sys.version_info.major != 2,
+                        reason='python2 specific, non-comparable test')
+    def test_richcmp_py2(self):
+        class Rich(object):
+            """Dummy class with traditional rich comparison support."""
+            def __lt__(self, other):
+                raise NotImplementedError('__lt__')
+            def __le__(self, other):
+                raise NotImplementedError('__le__')
+            def __eq__(self, other):
+                raise NotImplementedError('__eq__')
+            def __ne__(self, other):
+                raise NotImplementedError('__ne__')
+            def __gt__(self, other):
+                raise NotImplementedError('__gt__')
+            def __ge__(self, other):
+                raise NotImplementedError('__ge__')
+
+        class CythonRich(object):
+            """Dummy class with spoof cython rich comparison support."""
+            def __richcmp__(self, other):
+                """
+                This method is never called. However it is introspected
+                by the cftime.datetime.__richcmp__ method, which will then
+                return NotImplemented, causing Python to call this classes
+                __cmp__ method as a back-stop, and hence spoofing the
+                cython specific rich comparison behaviour.
+                """
+                pass
+            def __cmp__(self, other):
+                raise NotImplementedError('__richcmp__')
+
+        class Pass(object):
+            """Dummy class with no rich comparison support whatsoever."""
+            pass
+
+        class Pass___cmp__(object):
+            """Dummy class that delegates all comparisons."""
+            def __cmp__(self, other):
+                return NotImplemented
+
+        # Test LHS operand comparison operator processing.
+        for op, expected in [(operator.gt, '__lt__'), (operator.ge, '__le__'),
+                             (operator.eq, '__eq__'), (operator.ne, '__ne__'),
+                             (operator.lt, '__gt__'), (operator.le, '__ge__')]:
+            with self.assertRaisesRegexp(NotImplementedError, expected):
+                op(self.date1_365_day, Rich())
+
+            with self.assertRaisesRegexp(NotImplementedError, '__richcmp__'):
+                op(self.date1_365_day, CythonRich())
+
+        # Test RHS operand comparison operator processing.
+        for op in [operator.gt, operator.ge, operator.eq, operator.ne,
+                   operator.lt, operator.le]:
+            with self.assertRaisesRegexp(TypeError, 'cannot compare'):
+                op(Pass(), self.date1_365_day)
+
+            with self.assertRaisesRegexp(TypeError, 'cannot compare'):
+                op(Pass___cmp__(), self.date1_365_day)
 
 
 class issue17TestCase(unittest.TestCase):