Adds internal Calendar Rata Die format for conversions between Calendars #5841
Conversation
|
Thank you @Qqwy! This is a large PR, so I will first talk about the base problems and then we can discuss specifics as we move forward.
Finally, you are implementing the conversion to gregorian epoch... couldn't you use the functions in |
Astutely observed. What happens on e.g. https://github.com/Qqwy/elixir/blob/a5ad3a77dc3d1dae6de91a9dd875d1904b60e9da/lib/elixir/lib/calendar/iso.ex#L81 is indeed wrong; and (hard-coded) assumes that DateTime and NaiveDateTime follow Calendar.ISO implicitly. This should be fixed, although I am not entirely sure in what way (Build the structures manually instead?). However, isn't the general problem that Date, DateTime and Time are not supposed to refer to Calendar.ISO, but rather to their own stored calendar module? I think this is a core issue that arises from the fact that these modules currently have multiple responsibilities (a between-calendar reusable struct, helpers to create Calendar.ISO (Date)(Time)s, conversion between Erlang <-> Calendar.ISO (Date)(Time)s).
The issue is that time is not always represented as hour, minute and seconds. Even if we restrict it to only h/m/s-like calendars, the amount of hours, minutes and seconds per day is not the same across calendars, and the start-of-day moment also is not always the same. Examples: The Hebrew calendar, the Hindu calendar, the Chinese calendar. We can convert between times by using a day fraction as intermediate structure, which is always relative to midnight. There is not a problem here, because even if we 'roll over' to a different day in the target calendar when converting, this day information is discarded as we're only interested in the time. However, when using dates, it is important to know when on the dates you are looking at, exactly because the different moments when a day starts. Time is cyclic, while dates are on a line.
It does not really limits the use of Rata Die. The idea is that if you want to convert dates, you'll need to pick a moment, say midnight, as reference time for the conversion. For calendars that use the same start-of-day moment, whatever time you pick does not matter as the result will always be unambiguous. For calendars that differ, however, the picked time moment is important. We could choose to create a function that converts dates by implicitly using 'midnight' as reference time, but this somewhat goes against the Explicit nature of Elixir. I'll draw a nice picture that explains why conversions between times and datetimes is possible, while conversions between dates is not better than words can.
Embarrassingly I have not at all thought to look in
Don't worry; the documentation could be improved semantically as well, I'm sure. I wanted to get this out quickly as to resume the discussion. My inner perfectionist will resolve these in due time 😋 . |
|
Here are the images to show the difference between converting dates, times and datetimes:
As can be seen, converting dates cannot be done: Converting 2016-01-12 (Gregorian) to the Hebrew Calendar could be either 2 Shvat 5776 or 3 Shvat 5776, depending on if the moment of the day is before/after sunset. However, if we do know the time as well, we can convert, as is the case when converting 2016-01-15T20:00:00Z to 6 Shvar 5776 (1 hour, 0 halakim)
|
lib/elixir/lib/calendar.ex
Outdated
| @@ -1337,6 +1376,12 @@ defmodule NaiveDateTime do | |||
|
|
|||
| ## Helpers | |||
|
|
|||
| defp discard_timezone_info(naive_datetime) do | |||
There was a problem hiding this comment.
Is this function ever used?
There was a problem hiding this comment.
You are completely correct. This indeed is a function that is not used anymore in the current version. I'll remove it.
Glad you noticed it! 😄
lib/elixir/lib/integer.ex
Outdated
| Runs both `div/2` and `mod/2` on the same input, and returns a tuple with both results. | ||
| """ | ||
| @spec div_mod(integer, neg_integer | pos_integer) :: integer | ||
| def div_mod(x, y) when is_integer(x) and is_integer(y) do |
There was a problem hiding this comment.
This is only used in Calendar.ISO module, so it should be a private function there.
lib/elixir/lib/integer.ex
Outdated
| 6 | ||
| """ | ||
| @spec gcd(integer, integer) :: non_neg_integer | ||
| def gcd(a, 0), do: abs(a) |
They should return tuples which DateTime will use to build its representation then.
Also yes. I would still say we should avoid the cyclic dependency. And the Calendar.ISO has always meant to be a temporary limitation (which this PR will hopefully help address).
This makes perfect sense, thank you. The image is also extremely helpful. However, according to the image, we should be able to convert from a NaiveDateTime to another NaiveDateTime in another calendar, shouldn't we? To sum up:
|
Yes! 😄
Depending on if you want There are quite probable other situations in which it would be useful as well; let's not be too quick to remove it.
Two NaiveDateTimes might not be in the same timezone, meaning that they are skewed by an unknown amount relative to another. Converting is thus only possible if we know the difference between their timezones.
The UTC-relative timezones as specified with Maybe an idea would be to add a 'metadata' map to the Rata Die format, in which calendars can supply meta-information that might be applicable to the conversion target calendar or not, such that things like the time zone and the requested microsecond precision can be kept iff both calendars support this. Let me ask some questions to you and the other Elixir Core members in return:
|
|
By the way, it might help to look at how Joda-Time, Java's de facto timekeeping implementation that supports eight calendars was implemented. Its 'key concepts' documentation is very readable, in my opinion. What Joda-time does good in my opinion:
What it lacks:
It's also interesting to note that it thus looks like it is possible to support at least these eight calendars without rigorously changing the fields of the existing structs, and without working with non-ISO8601 timezones. Their (Date)(Time) objects use the 'decorator pattern', which is analogous of passing a |
We can't remove them before 2.0 anyway. :)
I believe this assumption doesn't fit However, your later answer on DateTime and timezones being something specific to ISO explains why calendar conversions wouldn't work for NaiveDateTime.
The trade-off we took here is that we would allow to support multiple calendars as long as they fit the regular hour-minute-second and year-month-date representations. We preferred to optimize for the most common scenario, both in terms of simplicity of use and performance, at the cost of limiting some usage. A completely generic API though can still be built as a library that would use protocols instead. Therefore we don't plan to support a more general representation. Therefore the last question is: are the current trade-offs enough to support other calendars or should we assume that we can only effectively support Calendar.ISO and consider the |
I think that maybe looking at what is possible with Joda-Time might shed some light on this.
Take your pick: You can still decide to put the burden on the user that the times are of the same zone or not in that case. It's possible to reason both ways here. Maybe the best idea to my own question (2.) I can come up with so far is to have Idea for changes to Elixir would then be:
|
If it requires a specific timezone information to properly convert to another calendar, then I believe we should not add it to NaiveDateTime. About your proposed changes, let's NOT do the following:
Let's NOT do this because we can add a field any time if we find it truly necessary. We should be conservative when adding because it is much harder to remove.
Let's add support for custom calendar as second argument but let's not provide an application wide default (yet). Although I don't think the second argument would be necessary for the |
|
Edit: this proposal is a bad idea™. :) About the Calendar API, can we break it into those four functions? @callback date_to_gregorian_days(year, month, day) :: integer()
@callback gregorian_days_to_date(integer()) :: {year, month, day}
@callback time_to_day_fraction(hour, minute, seconds, microseconds, offset) :: day_fraction
@callback day_fraction_to_time(day_fraction) :: {hour, minute, second, microseconds}
@type day_fraction :: {integer(), pos_integer()}Note the first element of I believe the above the smallest API we can require from Calendar while still allowing Time and DateTime to build on top of it (and even NaiveDateTime if we find so necessary in the future). |
|
Actually, please ignore my proposal above. It won't work for datetime as we need to pass both date and time. I believe your current proposal looks good, we just need to make it accept multiple arguments and return tuples instead of the DateTime structs. |
|
Agreed on all fronts! Expect a refined implementation (including functions that work with tuples instead of maps to resolve the cyclic dependencies) that does these things on 2017-03-09 (Gregorian 😜 ). |
Raising the only way to go due to backwards compat, yet.
The thing about Date.compare/2 is that it is reasonable to ask if a DateTime is at a given Date ignoring the other fields. I cannot find a good reason to convert to to_erl directly or to another calendar transparently. In this case calling DateTime.to_date first doesn't hurt IMO. |
|
So in the case of
This feels very weird/confusing. |
|
Oh, let's keep the special ISO clause and we can add a deprecation warning.
--
*José Valimwww.plataformatec.com.br
<http://www.plataformatec.com.br/>Founder and Director of R&D*
|
…to_erl, Date.to_iso8601, Time.to_iso8601. Passing DateTimes or NaiveDateTimes to these functions directly is now deprecated.
|
Deprecation warnings have been added. As the proper behaviour has been part of Elixir for as long as the Date/Time types have, a warning is shown right away. The CHANGELOG and the documentation of these functions has been updated to reflect the changes. |
CHANGELOG.md
Outdated
| * [Kernel] `not left in right` is soft-deprecated in favor of `left not in right` | ||
|
|
||
| ### 4. Deprecations | ||
|
|
||
| #### Elixir | ||
|
|
||
| * [Calendar] Calling `Date.to_erl/1`, `Date.to_iso8601/1`, `Time.to_erl/1` and `Time.to_iso8601/1` directly with a `DateTime` or `NaiveDateTime` struct is deprecated in favour of converting them to `Date`s/`Time`s first by using `DateTime.to_date/1`, `NaiveDateTime.to_date/1`, `DateTime.to_time/1` and `NaiveDateTime.to_time/1` respectively. |
There was a problem hiding this comment.
Please don't change the CHANGELOG. We will do it after merging (and it will also avoid your PR getting conflicts!)
|
The only thing that is not yet done is changing
I am leaning towards the former of these two, because it might be counter-intuitive if |
|
@Qqwy I agree. It should be the date+time of now. |
|
I think it is time for a new review round :-) |
|
❤️ 💚 💙 💛 💜 |
|
Thank you so much @Qqwy, there are some minor adjustments but those can be fixed locally. The conceptual changes have all been covered. If something is missing, please send more pull requests. If you would like to work on removing the calendar dependency, that will be welcome, as it would allow us to use also the calendar library in other projects such as elixirscript. :) |
|
Wonderful! 😍 Thank you so much. It is a joy to work with you 😃 . |
| end | ||
| end | ||
|
|
||
| def convert(%{calendar: calendar} = naive_datetime, calendar) do |
There was a problem hiding this comment.
@Qqwy We are missing documentation and spec for this function, can you please send a PR?
|
|
||
|
|
||
| @spec convert!(DateTime.t, Calendar.calendar) :: DateTime.t | ||
| def convert!(datetime, calendar) do |
There was a problem hiding this comment.
@Qqwy documentation is also missing here.
| end | ||
| end | ||
|
|
||
| @spec convert!(NaiveDateTime.t, Calendar.calendar) :: NaiveDateTime.t |
There was a problem hiding this comment.
@Qqwy documentation is also missing here.
| {:ok, result_time} | ||
| end | ||
|
|
||
| @spec convert!(Time.t, Calendar.calendar) :: Time.t |
There was a problem hiding this comment.
@Qqwy documentation is also missing here.
|
Yes, give me a minute! |
|
@Qqwy i have added two issues and copied you on them. :) |
This Pull Request is a follow-up from @kipcole9 's PR #5603 and the discussion in the Calendars & Calendar Conversions topic.
The current state of this PR is mainly to rekindle the discussions about this subject: There still are a few choices we must make before this functionality can be included into Elixir.
This checklist will be kept updated as the discussion in this topic evolves.
Checklist:
Calendar.day_fraction :: {non_neg_integer, pos_integer, extra: %{}}andCalendar.rata_die :: {integer, day_fraction, extra :: %{}}that are used as internal format to convert between times and datetimes, respectively. An in-day fraction is used to ensure that precision is always high enough.Calendar.datetime_to_rata_die(year, month, day, hour, minute, second, millisecond, time_zone, zone_abbr, utc_offset, std_offset)andCalendar.datetime_from_rata_die(rata_die).Calendar.time_to_day_fraction(hour, min, second, millisecond)andCalendar.time_from_day_fraction(day_fraction).DateTime.compare/2function that works across calendars by usingdatetime_to_rata_dieinstead ofto_unixas is now the case.DateTime.diff/2function that returns the difference between two datetimes (across calendars) in theCalendar.rata_dieformat.:calendarfield.@callback day_rollover_relative_to_midnight_utc() :: day_fractionto the Calendar behaviour, which is used to check ifDates andNaiveDateTimes (rather than onlyDateTimes) can be converted between two calendars.to_*so they dispatch on their containing calendar's datetime_to_rata_die, before converting to the external format.valid_date?,valid_time?,valid_datetime?,valid_naive_datetime?.DateTime.apply_tz_offset)Date.compare, Time.compare, DateTime.compare, NaiveDateTime.comparethat explain when you can/cannot compare across calendars.To be discussed:
valid_datetime?etc. to the Calendar behaviour?valid_time?,valid_date?,valid_datetime?,valid_naive_datetime?.This Pull Request will add the following to Elixir:
(This list is superseded by above checklist)
Calendar.day_fraction :: {non_neg_integer, pos_integer}andCalendar.rata_die :: {integer, day_fraction}that are used as internal format to convert between times and datetimes, respectively. An in-day fraction is used to ensure that precision is always high enough.Calendar.datetime_to_rata_die(datetime)andCalendar.datetime_from_rata_die(rata_die).Calendar.time_to_day_fraction(time)andCalendar.time_from_day_fraction(day_fraction). Note that these are not used by other code yet, as the%Time{}struct does not contain Calendar information. (See below).DateTime.compare/2function that works across calendars by usingdatetime_to_rata_dieinstead ofto_unixas is now the case.DateTime.diff/2function that returns the difference between two datetimes (across calendars) in theCalendar.rata_dieformat.Important to note:
Dates are by design not convertible across calendars. If you want to convert a date, you'll need to convert it to a DateTime (and thus pick some reference time to use). The reference time you pick matters, as not all calendars roll over to the next day on midnight (some do at noon, others at dusk, etc). Without specifying a reference time, the result might therefore be ambiguous. What reference time to pick is an explicit choice that the developer should make, in my opinion.NaiveDateTimesare not convertible for exactly the same reason: They will only be unambiguous if their timezone is specified.Other things I've stumbled upon and would like to note:
Timedoes not contain nor use Calendar information at all. I think it would be a good idea to add this (staying backwards compatible of course), because this implicit, hard-coded dependency on it using Calendar.ISO underwater means that constructing functions likeNaiveDateTime.new(date, time)do not make sense when used for other calendars. More importantly, it will make it possible to construct and compare times across calendars, etc, as Calendar can then dispatch to the proper Calendar behaviour based on the:calendarfield of the map passed to its functions.Calendar.ISO'sdatetime_to_rata_dieanddatetime_from_rata_diedo not handle leap seconds, as there is no way for Elixir to know about them. The only possibility for a calendar that does support leap seconds that I can think of, is a 3rd party package that reads its info from the:tzdatapackage. Calendar.ISO's conversions will be off by at most86400/864001th of a day during a leap second day. (And will be correct again after that day).NaiveDateTime.compare(left, right)andDate.compare(left, right)do not care if the calendars ofleftandrightare the same. This is something that should be fixed (regardless of whatever happens to the rest of this Pull Request). (EDIT: Date.compare/2, DateTime.compare/2, NaiveDateTime.compare/2 happily compare across calendars. #5842 was made)from_erl,to_erl,to_unix, etc. This means that these modules are taking care of three different concerns right now. For instance, DateTime is:%DateTime{}struct.We might therefore want to split these modules, into e.g.
DateTimeandCalendar.ISO.DateTime. But this will be somewhat problematic in terms of backwards compatibility. Any ideas?All help and feedback is greatly welcome! :-)