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

RFC: Deprecate Timex.compare and change Timex.diff to a signed integer result #137

Closed
tallakt opened this issue Mar 20, 2016 · 10 comments
Closed

Comments

@tallakt
Copy link

tallakt commented Mar 20, 2016

iex(25)> Timex.diff expiry, Timex.DateTime.now, :minutes
7
iex(26)> Timex.diff Timex.DateTime.now, expiry, :minutes
7

According to the docs, the result should be signed. Timex version 2.1.1

@tallakt tallakt changed the title Timex.DateTime.diff missign sign in result Timex.DateTime.diff missing sign in result Mar 20, 2016
@bitwalker
Copy link
Owner

@tallakt I believe the reason I changed this was because a signed diff didn't seem useful to me when one could use compare to determine the past/future relation. It's easy enough to change back, but I suppose my question is: do you rely on the signed result? Is it a useful property of the function compared to returning the diff as a non-negative integer always, and using compare to determine how two dates/datetimes relate to each other?

Another question then becomes, if diff returns a signed value, is compare even useful as a function? Should it be deprecated/removed and just leave diff as the sole means of comparison?

@tallakt
Copy link
Author

tallakt commented Mar 20, 2016

I would rather just use diff and then normal operators for comparing the results rather than the -1, 0, +1 result which feels alot like C++ to me. That's my opinion anyway. I don't feel very strongly about it, but as a minimum, we should update the docs.

@bitwalker
Copy link
Owner

Agreed on the docs. I'll think about these two functions in general and post back here with my decision. Chances are I will deprecate compare and just lean on diff for double duty moving forward, but I want to think it through before I decide.

@bitwalker
Copy link
Owner

I've updated the docs in the meantime

@bitwalker bitwalker changed the title Timex.DateTime.diff missing sign in result RFC: Deprecate Timex.compare and change Timex.diff to a signed integer result Mar 21, 2016
@bitwalker bitwalker self-assigned this Mar 21, 2016
@bitwalker bitwalker added this to the Version 2.2.0 milestone Mar 21, 2016
@tallakt
Copy link
Author

tallakt commented Mar 22, 2016

After giving it a bit of thought, it is probably more intuitive that dates arithmetic with dates behave like with normal numbers, with signed results, eg:

iex> Timex.diff a, b
4
iex> Timex.diff b, a
-4

you could always just use abs if you need the absolute distance

@bitwalker
Copy link
Owner

@tallakt Agreed. I think I'll make that change sooner rather than later, because I don't want the current behaviour to become too entrenched.

@myronmarston
Copy link

FWIW, I'm working on upgrading to 2.0 and the fact that diff is no longer signed (compared to 0.x and 1.x) is giving me trouble. I've implemented our own diff function that delegates to Timex.diff and restores the sign based on compare but it feels like I shouldn't have to do that. It would be like if a math library defined - to always return the abs value, so that 5 - 10 returned 5 (requiring the user to compare the values to figure out if it should be negative).

Anyhow, sounds like you are already planning and restoring the signed behavior but I wanted to lend my voice to it as well :).

@myronmarston
Copy link

By the way, the signed behavior of diff in Timex 0.x and 1.x worked the opposite of how I expected. In my mind, I thought of diff as the equivalent of subtraction for date math -- so just as 10 - 15 is negative, I expected diff(may_10_2016, may_15_2016, :days) to return -5 and diff(may_15_2016, may_10_2016, :days) to return 5 -- but it does the opposite. Reading the old docs for diff (from when it returned a sign value), it says:

Calculate time interval between two dates.

...which helps explain the behavior. I think that it's easy to assume that diff(a, b) is the equivalent of a -b so if you want to preserve this behavior, I'd suggest calling the function interval instead of diff. Or, if you're willing to flip the sign from how it behaved before, diff would still be a good name, IMO.

@bitwalker
Copy link
Owner

@myronmarston This has been merged into master and the behaviour is exactly as you describe it, putting an older date before a newer one returns a negative value, and vice versa. The same behaviour as compare.

@bitwalker
Copy link
Owner

This RFC has been merged in 3.0, so the diff behaviour is again signed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants