[dfdatetime] Added date time values compare functions

[dfdatetime] Added date time values compare functions

[Joachim Metz, 2018-02-18 17:01:09]

Code updated.

[onager, 2018-02-19 14:22:30]

https://codereview.appspot.com/340290043/diff/20001/dfdatetime/cocoa_time.py
File dfdatetime/cocoa_time.py (right):

https://codereview.appspot.com/340290043/diff/20001/dfdatetime/cocoa_time.py#...
dfdatetime/cocoa_time.py:49: January 1, 1970.
+or None if the timestamp value is not set
+00:00 on January 1, 1970

Also, for commentary, this includes whatever fractional second value is that's
available in the timestamp, right?

https://codereview.appspot.com/340290043/diff/20001/dfdatetime/interface.py
File dfdatetime/interface.py (right):

https://codereview.appspot.com/340290043/diff/20001/dfdatetime/interface.py#n...
dfdatetime/interface.py:51: other_normalized_timestamp = getattr(other,
'_normalized_timestamp', None)
_GetNormalizedTimestamp already returns None if it's not available, why not just
call the method? This also seems strange, because _GetNormalizedTimestamp needs
to have been called for other at some point as well, and I don't see any
guarantees about that having happenend - it's not called in __init__ for
example.

Doing this that way will also crash if there's an attempted comparison with
something else, like a python date or an int, which is desirable IMO.

This goes for the other usage of this pattern, too.

https://codereview.appspot.com/340290043/diff/20001/dfdatetime/semantic_time.py
File dfdatetime/semantic_time.py (right):

https://codereview.appspot.com/340290043/diff/20001/dfdatetime/semantic_time....
dfdatetime/semantic_time.py:186: self._sort_order = 99
Shouldn't "never" be sorted after all other timestamps? I'd expect to see things
like never expiring settings at the end of a timeline, for example.

https://codereview.appspot.com/340290043/diff/20001/tests/interface.py
File tests/interface.py (right):

https://codereview.appspot.com/340290043/diff/20001/tests/interface.py#newcode19
tests/interface.py:19: date_time_values1 = interface.DateTimeValues()
Please add a test that involves _GetNormalizedTimestamp being called in
comparisons.

https://codereview.appspot.com/340290043/diff/20001/tests/semantic_time.py
File tests/semantic_time.py (right):

https://codereview.appspot.com/340290043/diff/20001/tests/semantic_time.py#ne...
tests/semantic_time.py:30: self.assertTrue(semantic_time_object1 !=
semantic_time_object2)
Please add test for the subclasses too - Invalid < Never etc.

[Joachim Metz, 2018-02-19 19:32:47]

https://codereview.appspot.com/340290043/diff/20001/dfdatetime/cocoa_time.py
File dfdatetime/cocoa_time.py (right):

https://codereview.appspot.com/340290043/diff/20001/dfdatetime/cocoa_time.py#...
dfdatetime/cocoa_time.py:49: January 1, 1970.
On 2018/02/19 14:22:30, onager wrote:
> +or None if the timestamp value is not set
> +00:00 on January 1, 1970
> 
> Also, for commentary, this includes whatever fractional second value is that's
> available in the timestamp, right?

Done.

https://codereview.appspot.com/340290043/diff/20001/dfdatetime/interface.py
File dfdatetime/interface.py (right):

https://codereview.appspot.com/340290043/diff/20001/dfdatetime/interface.py#n...
dfdatetime/interface.py:51: other_normalized_timestamp = getattr(other,
'_normalized_timestamp', None)
> _GetNormalizedTimestamp already returns None if it's not available, why not
just call the method?

left over from first iteration.

> This goes for the other usage of this pattern, too.

So Python actually allows 'a' != 1, but I agree raising ValueError for
non-DateTimeValue objects is cleaner.

https://codereview.appspot.com/340290043/diff/20001/dfdatetime/semantic_time.py
File dfdatetime/semantic_time.py (right):

https://codereview.appspot.com/340290043/diff/20001/dfdatetime/semantic_time....
dfdatetime/semantic_time.py:186: self._sort_order = 99
Yes I was thinking about implementing this in a follow up CL not to make this
one too large, but I'll include it in this one since you mention it.

https://codereview.appspot.com/340290043/diff/20001/tests/interface.py
File tests/interface.py (right):

https://codereview.appspot.com/340290043/diff/20001/tests/interface.py#newcode19
tests/interface.py:19: date_time_values1 = interface.DateTimeValues()
Not sure what you mean, but I've changed things a bit

https://codereview.appspot.com/340290043/diff/20001/tests/semantic_time.py
File tests/semantic_time.py (right):

https://codereview.appspot.com/340290043/diff/20001/tests/semantic_time.py#ne...
tests/semantic_time.py:30: self.assertTrue(semantic_time_object1 !=
semantic_time_object2)
On 2018/02/19 14:22:30, onager wrote:
> Please add test for the subclasses too - Invalid < Never etc.

Acknowledged.

[Joachim Metz, 2018-02-19 20:07:07]

Code updated.

[onager, 2018-02-20 13:04:02]

Some minor nits, LGTM

https://codereview.appspot.com/340290043/diff/40001/dfdatetime/semantic_time.py
File dfdatetime/semantic_time.py (right):

https://codereview.appspot.com/340290043/diff/40001/dfdatetime/semantic_time....
dfdatetime/semantic_time.py:59: """Determines if the date time values are
greater equal than other.
"greater than or equal to"

https://codereview.appspot.com/340290043/diff/40001/dfdatetime/semantic_time....
dfdatetime/semantic_time.py:99: """Determines if the date time values are less
equal than other.
"less than or equal to"

https://codereview.appspot.com/340290043/diff/40001/dfdatetime/semantic_time....
dfdatetime/semantic_time.py:243: """Determines if the date time values are
greater equal than other.
"greater than or equal to"

https://codereview.appspot.com/340290043/diff/40001/dfdatetime/semantic_time....
dfdatetime/semantic_time.py:277: """Determines if the date time values are less
equal than other.
"less than or equal to"

https://codereview.appspot.com/340290043/diff/40001/tests/semantic_time.py
File tests/semantic_time.py (right):

https://codereview.appspot.com/340290043/diff/40001/tests/semantic_time.py#ne...
tests/semantic_time.py:123: semantic_time_object1 = semantic_time.Never()
rename these to never_1 and never_2, calling them other things in a test for the
Never class is makes this much more difficult to follow.

[Joachim Metz, 2018-02-22 09:32:44]

Code updated.

[Joachim Metz, 2018-02-22 09:38:31]

https://codereview.appspot.com/340290043/diff/40001/dfdatetime/semantic_time.py
File dfdatetime/semantic_time.py (right):

https://codereview.appspot.com/340290043/diff/40001/dfdatetime/semantic_time....
dfdatetime/semantic_time.py:59: """Determines if the date time values are
greater equal than other.
On 2018/02/20 13:04:01, onager wrote:
> "greater than or equal to"

Done.

https://codereview.appspot.com/340290043/diff/40001/dfdatetime/semantic_time....
dfdatetime/semantic_time.py:99: """Determines if the date time values are less
equal than other.
On 2018/02/20 13:04:01, onager wrote:
> "less than or equal to"

Done.

https://codereview.appspot.com/340290043/diff/40001/dfdatetime/semantic_time....
dfdatetime/semantic_time.py:243: """Determines if the date time values are
greater equal than other.
On 2018/02/20 13:04:01, onager wrote:
> "greater than or equal to"

Done.

https://codereview.appspot.com/340290043/diff/40001/dfdatetime/semantic_time....
dfdatetime/semantic_time.py:277: """Determines if the date time values are less
equal than other.
On 2018/02/20 13:04:01, onager wrote:
> "less than or equal to"

Done.

https://codereview.appspot.com/340290043/diff/40001/tests/semantic_time.py
File tests/semantic_time.py (right):

https://codereview.appspot.com/340290043/diff/40001/tests/semantic_time.py#ne...
tests/semantic_time.py:123: semantic_time_object1 = semantic_time.Never()
On 2018/02/20 13:04:01, onager wrote:
> rename these to never_1 and never_2, calling them other things in a test for
the
> Never class is makes this much more difficult to follow.

Done.

[Joachim Metz, 2018-02-22 09:40:28]

Code updated.

[Joachim Metz, 2018-02-22 10:13:14]

Changes have been merged with master branch. To close the review and clean up
the feature branch you can run: review.py close compare1