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

trunc/floor/ceil/round interface inconsistency for methods involving dates? #18574

Closed
Sacha0 opened this issue Sep 18, 2016 · 4 comments
Closed
Labels
dates Dates, times, and the Dates stdlib module

Comments

@Sacha0
Copy link
Member

Sacha0 commented Sep 18, 2016

trunc methods typically conform to trunc([T,] x, [digits, [base]]) --- except where they involve Dates/DateTimes: trunc methods that accept a date type / value pair take the form trunc(x, T) rather than trunc(T, x). Should these non-conforming methods conform? Apologies if I missed a good reason for this inconsistency while searching, and please close if so. Best!

Edit: The same holds for at least some floor, ceil, and round methods involving dates.

@Sacha0 Sacha0 changed the title trunc interface inconsistency for methods involving dates? trunc/floor/ceil/round interface inconsistency for methods involving dates? Sep 19, 2016
@simonster simonster added the dates Dates, times, and the Dates stdlib module label Sep 19, 2016
@tkelman
Copy link
Contributor

tkelman commented Sep 20, 2016

Good catch. @spurll any reason for having the arguments this way in #17037 ? Must've missed that in review.

@spurll
Copy link
Contributor

spurll commented Sep 20, 2016

I modelled the function signatures for floor, ceil, and round for dates on trunc, which already existed. But I think that this is not actually an issue, because trunc(T, x) truncates, converting the result to type T. For dates that doesn't make sense: trunc(dt, Minute) doesn't return a Minute value, it still returns a Date/DateTime.

For this reason, when we're looking at trunc(dt::TimeType, ::Type{Period}), the type in question is really more analogous to digits in trunc([T,] x, [digits, [base]]) rather than T. It's also worth considering that for the rounding functions it is permissible to pass in a Period value, not just a Period type (e.g., Dates.Minute(15)), making the analogy to digits even more apt.

So I think the existing signature is sensible for two reasons: (1) it reads the way one would say it (e.g., round(dt, Minute(15)) would be "round dt to fifteen minutes") and (2) I see the period to which we're rounding as more analogous to digits in the trunc example above, rather than T.

@simonbyrne
Copy link
Contributor

The idea is that trunc(T,x) should be equivalent to convert(T,trunc(x)), which is not the case for dates, so I think the existing behaviour is fine.

@Sacha0
Copy link
Member Author

Sacha0 commented Sep 20, 2016

Much thanks for the lucid explanation @spurll! Best!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dates Dates, times, and the Dates stdlib module
Projects
None yet
Development

No branches or pull requests

5 participants