zopefoundation / DateTime

This package provides a DateTime data type, as known from Zope. Unless you need to communicate with Zope APIs, you're probably better off using Python's built-in datetime module.
Other
19 stars 25 forks source link

Fix compare methods to None (fix #52). #53

Closed fdiary closed 1 year ago

fdiary commented 1 year ago

This is a fixup commit of 'further py3 work' that changed the behaviour of compare methods to None. Now None is less than any DateTime instance, just same as DateTime 2.

fdiary commented 1 year ago

@dataflake thanks for your review !

I would like to squash all changes here into one commit. Shall I do it and push force here, or you / any maintainer will do ?

dataflake commented 1 year ago

When a PR has been reviewed the PR author does the merge.

icemac commented 1 year ago

@fdiary Coverage fell below the previous value, could you please have a look what have might cause this drop?

fdiary commented 1 year ago

@fdiary Coverage fell below the previous value, could you please have a look what have might cause this drop?

Thanks for your comment.

Before this change, equalTo(None) is same as equalTo(0) (i.e. not 0.0) thus the following part was evaluated, that is no longer the case.

    def equalTo(self, t):
        """Compare this DateTime object to another DateTime object
        OR a floating point number such as that which is returned
        by the python time module.

        Returns true if the object represents a date/time equal to
        the specified DateTime or time module style time.

        Revised to give more correct results through comparison of
        long integer microseconds.
        """
        ...
        if isinstance(t, float):
            return self._micros == long(t * 1000000)
        try:
            return self._micros == t._micros
        except AttributeError:
            return self._micros == t   # <-- this line is evaluated if t is int.

So the following additional assertions will bring back the same coverage in src/DateTime/DateTime.py.

--- src/DateTime/tests/test_datetime.py
+++ src/DateTime/tests/test_datetime.py
@@ -218,6 +218,16 @@ class DateTimeTests(unittest.TestCase):
         self.assertTrue(dt.notEqualTo(dt1))
         self.assertFalse(dt.equalTo(dt1))

+    def test_compare_methods_int(self):
+        # Compare a date to int
+        dt = DateTime('1997/1/1')
+        self.assertTrue(dt.greaterThan(0))
+        self.assertTrue(dt.greaterThanEqualTo(0))
+        self.assertFalse(dt.lessThan(0))
+        self.assertFalse(dt.lessThanEqualTo(0))
+        self.assertTrue(dt.notEqualTo(0))
+        self.assertFalse(dt.equalTo(0))
+
     def test_compare_methods_none(self):
         # Compare a date to None
         for dt in (DateTime('1997/1/1'), DateTime(0)):

But according to the interface documents, only DateTime or float instance is expected as an argument here. In my opinion, int argument should be rather casted to float (and we should remove try-except). Here is the current behaviour, that does not look good.

>>> DateTime(1.0).equalTo(1.0)
True
>>> DateTime(1).equalTo(1)
False # oh...

Anyway it will be a different topic than this PR. I can raise another PR to fix behaviour for int if you prefer.

icemac commented 1 year ago

@fdiary Thank you for your detailed analysis. Let's fix the int problem in another PR and merge that new PR first to get back the required coverage here.

fdiary commented 1 year ago

@fdiary Thank you for your detailed analysis. Let's fix the int problem in another PR and merge that new PR first to get back the required coverage here.

Done, #54

dataflake commented 1 year ago

IMHO this is ready for merging

fdiary commented 1 year ago

Thanks for your review !

icemac commented 1 year ago

Released in https://pypi.org/project/DateTime/5.2/.