-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Implement Duration and shift/2 for calendar types #13385
Conversation
Hi folks, I have been thinking about this and, the more I think about it, the more I wonder if we should instead introduce a proper Duration type. Ecto could benefit from such a type. As well as Explorer. The Duration module could also provide regular arithmetic operations between durations which are well defined (+, - and *), it could provide a protocol for applying the duration to any data type, as well as the ability to provide random ranges (aka intervals) given a start value and a duration (ranges can either be multiplicative or additive). The specification of durations can also be according to the iso standard: https://en.wikipedia.org/wiki/ISO_8601#Durations, including a ~P sigil. Although I would skip floating points and weeks from the initial implementation. It probably makes sense to first scratch this itch as a library though. Thoughts? |
Yes please. I implemented Cldr.Duration which might help kick-start things. I also have a full ISO8601 duration parser as part of Tempo in case thats useful to anyone. |
Hi @tfiedlerdejanze. After more pondering, I think we should go with a duration datatype. The datatype will provide a general abstraction, including ranges, that works with several datatypes. So instead of adding several functions to four modules (Date, Time, NaiveDateTime, DateTime), we can add them to a single one. In any case, I wanted to thank you for the PR. We have been thinking about adding Date.shift for quite a while and, once we see the code, it really helps move the conversation forward and make us ask the right questions. This would not have happened without your work. ❤️ |
Thank you @josevalim and @kipcole9, appreciate the reviews and thoughts made around that topic! I like the idea of the Duration type, and actually have needed this sort of shift behaviour mostly while working with structures like Postgrex.Interval, which appears to close the loop to that idea :) I am wondering about what the Duration api would actually look like though. One of the gripes i had with libraries implementing similar shift behaviours, was that they exposed a single interface to receive and return the same date/time struct, which often makes it hard to judge with what kind of date/time struct one is working with. Sorry for all the questions, if you were open to discuss the topic elsewhere in more detail, or maybe even share part of the vision, i'd love to contribute where possible! |
I think your point on API is valid, and maybe that's argument to add a
But the output type is indeed clearer. Regarding positive/negative, I found this discussion to be interesting: moment/moment#2408. It seems to be a common needed and other libraries made extensions to allow it. Perhaps a better way to frame it is: Duration will give us a better foundation. But we might still add |
Thanks for sharing that, glad to read it's generally supported!
This is actually the most important to me and one of the many things the standard library really shines at. Daring suggestion, how about moving towards supporting
In parallel to the above, the Duration protocol could be refined + defined to then partially replace the shift implementations as they already work with Duration.t() |
Unfortunately I am worried this may leave us with an outdated or wrong API. For example, I brought we may need to return |
That's my suggestion in 2). It'd be
in |
You are definitely welcome to give it a shot but I just want to be clear that it is unlikely we would merge it at this stage. But I agree that we are all likely to learn from it. For example, just in the proposal above, I can think of:
|
Given the plan would be to add the
For consistency with the
imho weeks on Duration could be supported. It should be up to the calendars and/or other implementations building on top of Duration to decide if they support it or not. |
Ok, let me slightly rephrase the question. Imagine we add a ~P sigil to follow ISO. Should this be valid?
Or should that be the same as:
That would be confusing in my opinion unless the huge majority of types can handle weeks properly, which @kipcole9 would be able to tell us. But I'd think so. :) |
Time scale unit values of 0ISO 8601 is strangely silent on the issue of
Therefore I think that Support
|
Yes, sorry i misread your initial question actually, unset fields should be treated as zero :) thanks for these elaborate notes @kipcole9 especially on the topic of weeks! |
Appreciate the review @sabiwara! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @tfiedlerdejanze!
This looks great and we got the go ahead to merge it. 🎉
I have dropped more suggestions regarding the docs. I also think we are missing two things in the docs:
-
We should explicitly mention that we round down. We have examples, but we should also mention it, and call it out in the example like we did for leap years by adding a tiny comment on top
-
We should have at least one example with a negative unit :) you can change one of the existing ones in each data type to be negative
Once those are in place and comments have been resolved, we can ship it!
Thanks for the suggestions and reviews everyone, all were very helpful and appreciated! I think I addressed all the latest comments, feel free to take another look. I also have a couple more PRs in the pipeline to
that I am looking forward to discuss! :) |
Just as a heads up. We will definitely accept the consolidation (thank you!), but there is no consensus on sigil ~P at the moment. I think the ISO syntax is not helpful to be made into a sigil and we are divided between a natural language one. We should probably wait for those until we use the durations more in practice, and see if a sigil is really necessary. I just dropped a comment on how we can improve the default inspect implementation. |
Thank you @tfiedlerdejanze for your patience and delivering this important feature! 💚 💙 💜 💛 ❤️ |
https://groups.google.com/g/elixir-lang-core/c/-el4xZbA9sw
Preface
We currently have
add/2-3
to manipulate calendar types in the standard library. These functions allow adding a specified amount of time of given unit to a date/time. The standard library currently misses means to apply more complex, or logical durations to calendar types. e.g. adding a month, a week, or one month and 10 days to a date.Reasons for it
While similar functionality exists in libraries, such as CLDR, Timex, Tox, adding this functionality to the standard library has already been requested and discussed at multiple occasions over the past years. To list a few examples:
Furthermore the shift behaviour in the extremely popular library Timex changed in Elixir >= 1.14.3 which may have complicated the mostly lean and non-breaking language upgrade Elixir has to offer.
Elixir has a great set of modules and functions that deal with date and time, the APIs are consistent and
shift/2-3
should fit right in, solving many standard needs of various industries, be it for reporting, appointments, events, scheduling, finance... the list goes on, engineers probably face the need to shift time logically more often than not in their careers.shift/2-3
integrates with the calendar behaviour, enabling libraries to adapt the shift behaviour for non-iso calendarsshift/2-3
often include much more functionality that isn't needed in most projects. This complicates the search to find the right library for the project and contributes to "dependency bloat"Technical details
Duration
A date or time must be shifted by a duration. There is an ISO8601 standard for durations, which the initial implementation is loosely following. The structure of a Duration lives in its own module with its own set of functions to create and manipulate durations. One example of where it diverts from the ISO standard, is that it implements microseconds. Microseconds in a duration are stored in the same format as in the time calendar types, meaning they integrate well and provide consistency.
The new
Duration
type comes with a set of arithmetic functions:add/2
,subtract/2
,multiply/2
,negate/1
. Up to discussion if we needdivide/2
as well.As of time of the proposal, duration units are intentionally
singular
. While not uncommon to use the plural form ofyears
,days
, etc. in the context of durations, we want to be consistent with the definition of time units in Elixir in general. These are singular as used inNaiveDateTime.add/3
or defined in types likeSystem.time_unit
Shift
The shift behaviour is implemented as a callback on Calendar and supported by all calendar types: Date, DateTime, NaiveDateTime and Time. Date, Time and NaiveDateTime each have their own implementation of a "shift", while DateTime gets converted to a NaiveDateTime before applying the shift, and is then rebuilt to a DateTime in its original timezone.
shift/2-3
also has guaranteed output types (which isn't a given in many libraries) and follows the consistent API which is established in the calendar modules.Benchmarks
Results
shift_bench.exs
Outlook
After adding the Duration type and shift behaviour to the standard library, the following things could be explored and derived from the initial work:
duration_to_seconds/1
andduration_from_seconds/1
.Duration.between/2
Duration.interval(~D[2000-01-01], month: 1)
, when iterated, would emit{:ok, date} | {:error, start, duration, reason}
entries~P[3 hours and 10 minutes]
or respecting the ISO format~P[T3H10M]
add/2-3
to reuse the calendar shift functionsDate.shift(~D[2021-02-28], [month: 1], round_month: true)
to result in~D[2021-03-31]
Reasons against it
While I am convinced that adding
shift/2-3
to the standard library would be very beneficial, nothing really speaks against the points mentioned above to be implemented in a library instead. However, something as crucial and central as date/time manipulation should still be part of the standard library, negating the risk of breaking changes, inconsistent behaviour and outdated or too unique ergonomics which aren't widely applicable, unlike what should be part of the standard library.