Comparisons aren't valid in relativedelta

[Ewan Higgs]

This bug has two parts

1. relativedelta.__eq__ doesn't check microseconds.

2. There is no comparison magic function in relativedelta which makes some comparisons return nonsense. e.g.

In [1]: import dateutil.relativedelta as r

In [2]: r.relativedelta(days=1) > r.relativedelta(days=2)
Out[2]: True

The __cmp__ I've submitted in the patch doesn't deal with the year/month/day/etc because I think it requires some discussion on what needs to be implemented there. But certainly 2 years > 1 years should be obviously correct.

[Tomi Hukkalainen (tpievila)]

Yeah, it is tricky... Which is greater, 29 days, or 1 month? I'm not sure if it makes sense to implement a partial solution, something that checks only days for example, as that can incorrectly imply that all cases are covered. In Python 3 when the comparisons are not implemented, the operation raises an exception. I'm inclined to just put magic functions that also raise NotImplemented, so that the functionality is equal.

In any case if/when a sensible solution is found, it needs to be implemented with rich comparisons, not with __cmp__ (it's not supported in Python 3).

[Tomi Hukkalainen (tpievila)]

Ugh, except that raising NotImplemented does not help. That would result in Python2 doing it's normal wonky "undefined but consistent" comparison.

There's just no way to say in Python 2 that two objects are not sortable.

[Ewan Higgs (ewan-higgs)]

I think you want to raise NotImplementedError; not NotImplemented.

I agree that there is some ambiguity between static times (days, hours, etc) and dynamic times (months, years). This already exists in __eq__ where 28 days is not equal to 1 month. It's also unclear what it means to add 1 month and 1 day to a datetime because it's not commutative.

One solution would be to assert that within a relativedelta static and dynamic times cannot be mixed within the same relativedelta. This would solve the ambiguity that exists between adding relativedelta(months=1, days=1) to datetime.datetime(2012, 2, 28): a month and a day could be added as distinct relativedelta objects to a datetime and it forces the user to explicitly set the ordering.

Going by this scheme, you could then raise NotImplementedError when comparing static and dynamic times, but still yield a sensible answer when comparing relativedeltas which contain static times.

Whether it makes sense to compare years and/or months can be explored at some point.

This could be a breaking change for some people. But I think it's an improvement because it doesn't seem to be reasonable to rely on relativedelta(months=1, days=1). If you think it's an good way to proceed let me know and I'll whip up a patch. I've implemented it in my forked version already.

[Ewan Higgs (ewan-higgs)]

>One solution would be to assert that within a relativedelta static and dynamic times cannot be mixed within the same relativedelta.

Ugh, I need an editor. I meant to say:

One solution would be to assert that static (days, hours, etc) and dynamic (years, months) times cannot be mixed within the same relativedelta.

[Tomi Hukkalainen (tpievila)]

> I think you want to raise NotImplementedError; not NotImplemented.

Well, as per the latter post, it doesn't change anything from simply
not defining the magic functions.

> I agree that there is some ambiguity between static times (days,
> hours, etc) and dynamic times (months, years). This already exists
> in __eq__ where 28 days is not equal to 1 month. It's also unclear
> what it means to add 1 month and 1 day to a datetime because it's
> not commutative.

It is perhaps not intuitively clear, but it is defined. See
http://labix.org/python-dateutil#head-72c4689ec5608067d118b9143cef6bdffb6dad4e.
Years are added first, then months, then days.

> One solution would be to assert that within a relativedelta static
> and dynamic times cannot be mixed within the same relativedelta.

This would be a severe limitation, I think.

> This would solve the ambiguity that exists between adding
> relativedelta(months=1, days=1) to datetime.datetime(2012, 2, 28): a
> month and a day could be added as distinct relativedelta objects to
> a datetime and it forces the user to explicitly set the ordering.

See above, the algorithm is not ambiguous. I agree that it's not
immediately clear, but it's also pretty obvious that there's no one
true way to calculate relativedeltas, so users probably (hopefully)
will check the exact behaviour from the docs, if it matters.

I think it's better to let users choose if they want to put the months
and days as separate objects.

> Going by this scheme, you could then raise NotImplementedError when
> comparing static and dynamic times, but still yield a sensible
> answer when comparing relativedeltas which contain static times.

Raising the error wouldn't change anything, Python2 would still
happily compare the two objects, with an undefined behaviour.

> This could be a breaking change for some people. But I think it's an
> improvement because it doesn't seem to be reasonable to rely on
> relativedelta(months=1, days=1). If you think it's an good way to
> proceed let me know and I'll whip up a patch. I've implemented it in
> my forked version already.

Nothing forces the users to rely on it. I do see the point about
comparisons being wonky, but this wouldn't actually help about that.

--
Tomi Pieviläinen, [phone]
A: Because it disrupts the natural way of thinking.
Q: Why is top posting frowned upon?

[Tomi Hukkalainen (tpievila)]

> I think you want to raise NotImplementedError; not NotImplemented.

Well, as per the latter post, it doesn't change anything from simply
not defining the magic functions.

> I agree that there is some ambiguity between static times (days,
> hours, etc) and dynamic times (months, years). This already exists
> in __eq__ where 28 days is not equal to 1 month. It's also unclear
> what it means to add 1 month and 1 day to a datetime because it's
> not commutative.

It is perhaps not intuitively clear, but it is defined. See
http://labix.org/python-dateutil#head-72c4689ec5608067d118b9143cef6bdffb6dad4e.
Years are added first, then months, then days.

> One solution would be to assert that within a relativedelta static
> and dynamic times cannot be mixed within the same relativedelta.

This would be a severe limitation, I think.

> This would solve the ambiguity that exists between adding
> relativedelta(months=1, days=1) to datetime.datetime(2012, 2, 28): a
> month and a day could be added as distinct relativedelta objects to
> a datetime and it forces the user to explicitly set the ordering.

See above, the algorithm is not ambiguous. I agree that it's not
immediately clear, but it's also pretty obvious that there's no one
true way to calculate relativedeltas, so users probably (hopefully)
will check the exact behaviour from the docs, if it matters.

I think it's better to let users choose if they want to put the months
and days as separate objects.

> Going by this scheme, you could then raise NotImplementedError when
> comparing static and dynamic times, but still yield a sensible
> answer when comparing relativedeltas which contain static times.

Raising the error wouldn't change anything, Python2 would still
happily compare the two objects, with an undefined behaviour.

> This could be a breaking change for some people. But I think it's an
> improvement because it doesn't seem to be reasonable to rely on
> relativedelta(months=1, days=1). If you think it's an good way to
> proceed let me know and I'll whip up a patch. I've implemented it in
> my forked version already.

Nothing forces the users to rely on it. I do see the point about
comparisons being wonky, but this wouldn't actually help with that.

[Elvis Pranskevichus (elprans)]

The missing check for microseconds in __eq__ is definitely a bug, regardless of other considerations in this ticket.

[Tom Hennigan (tomhennigan)]

Agreed, the microseconds bug is fairly obviously wrong, e.g. `relativedelta(seconds=1, microseconds=1) == relativedelta(seconds=1)`.

Being unable to compare relativedelta objects easily also annoyed me, and so I've also implemented a solution (didn't Google before, doh!). I implemented the rich comparison operators and added __cmp__ based on them. My solution currently is not taking leapseconds into account, that needs to be changed.

https://github.com/tomhennigan/python-dateutil/compare/feature;comparable-relativedelta

[Tom Hennigan (tomhennigan)]

Ewan Higgs (@ehiggs on GitHub) has updated my patch to allow correct comparison of relative only and non-relative only fields in relativedelta objects https://github.com/ehiggs/python-dateutil/compare/patches-for-1.5...ehiggs:feature/comparable-relativedelta