Skip to content

Time zone behaviour and functionality (RFC/WIP) #7914

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

Closed
wants to merge 24 commits into from

Conversation

lau
Copy link
Contributor

@lau lau commented Jul 16, 2018

This provides a behaviour for providing time zone data to Elixir (lib/elixir/lib/calendar/time_zone_database.ex). It allows anyone to implement a library that provides time zone data and use that with Elixir.

In addition to the behaviour there is code changes that enables Elixir to get a DateTime for the time right now in any timezone, to change between timezones and to create a new DateTime in any timezone. All using the behaviour defined by TimeZoneDatabase.

The behaviour includes functions for getting timezone information for a certain period for a certain time zone. Either by the time in UTC or the "wall time". Wall time being the time you would see on a clock on the wall, that is taking into account UTC offset as well as DST.

There are also functions for getting leap second information. This information is provided by IANA in the time zone data tar file. It is useful for validating UTC datetimes and for calculating differences where leap seconds are taken into account. It could also be used for calculating TAI and GPS time. In this PR there is currently not any functions that use the leap second data, but I might update the PR with that later.

Additionally there is functionality that uses the time zone data in date_time.ex.

The functions take an optional module name with a module that implement the behaviour:

iex> DateTime.from_naive(~N[2018-10-28 02:30:00], "Europe/Copenhagen", Tzdata.TimeZoneDatabase)
{:ambiguous,
 [#DateTime<2018-10-28 02:30:00+02:00 CEST Europe/Copenhagen>,
  #DateTime<2018-10-28 02:30:00+01:00 CET Europe/Copenhagen>]}

Instead of providing the module as a argument, Elixir can be configured to use a specific module.

:elixir_config.put(:time_zone_module, Tzdata.TimeZoneDatabase)

Then the argument is not needed for the functions and the configured module will be used instead:

iex> DateTime.from_naive(~N[2018-10-28 02:30:00], "Europe/Copenhagen")
{:ambiguous,
 [#DateTime<2018-10-28 02:30:00+02:00 CEST Europe/Copenhagen>,
  #DateTime<2018-10-28 02:30:00+01:00 CET Europe/Copenhagen>]}

gaps and overlaps (ambiguity) for instance during "spring forward" and autumn are handled.

DateTime.from_naive(~N[2018-10-28 05:30:00], "Europe/Copenhagen", Tzdata.TimeZoneDatabase)
{:ok, #DateTime<2018-10-28 05:30:00+01:00 CET Europe/Copenhagen>}

You can get the time right now in any timezone:

{:ok, datetime} = DateTime.now("Europe/London")
{:ok, #DateTime<2018-07-16 18:03:15.070876+01:00 BST Europe/London>}

Get the same time for a datetime in another timezone:

iex(6)> DateTime.shift_zone(datetime, "America/New_York")
{:ok, #DateTime<2018-07-16 13:03:15.070876-04:00 EDT America/New_York>}

This is not considered finished. Some of the details could be optimised. The main thing I focused on is the TimeZoneData behaviour. The iex> examples above were made with a working implementation of the behaviour not included here.

@michalmuskala
Copy link
Member

I really like where this is going 👏. Thank you for working on this! Here are couple design considerations when it comes to the time zone database interface - I haven't looked at the functions in DateTime too closely yet.

  • We should probably use the Calendar.iso_days abstraction instead of gregorian seconds in the interfaces - this will keep consistency with other elements of the calendar system and should limit required conversions.

  • The :elixir_config module is private - we should probably have some wrapper for it or just use the application env.

  • In the data structure returned from the database callbacks there are fields that are not used in the actual implementation. The implementation only uses the values of utc_offset, std_offset, and zone_abbr. I think we should keep the interface minimal and not include the other data, if we don't need it. An option could be also to use tuples instead of maps, but I'm not sure about that one - it might limit us on extensibility in the future.

  • Some functions, when information is lacking, return a list of possible values. Is it actually a list or can there always be just two values? If it's just two values, I think it should return something like {:gap, lower, higher} instead.

  • I think, by default, we might provide a database module that has only information for UTC and raises nice errors for other timezones. This should simplify some code paths.

  • I'm not sure about the leap_seconds callback. It seems to me that the information we want to obtain there is - "What is the difference between TAI and UTC at the specified point in time". If so, I think this function should receive a datetime value (probably in the "iso days" format) and return the integer offset. If we go with that approach, it might return: {:ok, non_neg_integer()} | {:incomplete_data, offset} or something similar, if requested for data too far into the future.

def from_naive(naive_datetime, time_zone)
@spec from_naive(NaiveDateTime.t(), Calendar.time_zone(), TimeZoneDatabase.t() | :from_config) ::
{:ok, t}
| {:ambiguous, [TimeZoneDatabase.time_zone_period()]}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be {:ambiguous, [t]} and {:gap, [t]}

@@ -177,20 +177,134 @@ defmodule DateTime do
Converts the given `NaiveDateTime` to `DateTime`.

It expects a time zone to put the NaiveDateTime in.
Currently it only supports "Etc/UTC" as time zone.

It only supports "Etc/UTC" as time zone if a TimeZoneDatabase
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would include backticks for autolinking in `TimezoneDatabase`


case by_wall(time_zone_data_module, time_zone, gregorian_seconds) do
{:single, period} ->
do_from_naive(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about from_naive_and_period(naive_datetime, time_zone_period), if time_zone is included in the time_zone_period map?

includes and begins from the begining of second 63594810000 and lasts until
just before second 63612954000.
"""
@type time_zone_period :: %{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would match the fields related to the time zone information in the DateTime struct: :zone_abbr, :utc_offset, :std_offset, and :time_zone.


@doc """
Takes a time zone name and a point in time for UTC and returns a
`time_zone_period` for that point in time.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you have to use t:time_zone_period/0 for linking types

@callback by_utc(Calendar.time_zone(), gregorian_seconds) ::
{:ok, time_zone_period} | {:error, :time_zone_not_found}

@doc """
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the first line should be a summary of the function.

| {:error, :time_zone_not_found}

@doc """
Returns a list of all known leap seconds. Each element in the list is a tuple
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add a new line after the first sentence, so Returns a list of all known leap seconds. becomes the summary.

from winter time to summer time, a tuple with `:gap` and a list of two time zone periods are returned. The first
period in the list is the period before the gap and the second period is the period just after the gap.

If there is only a single possible period for the provided `gregorian_seconds`, the a tuple with `:single`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo s/the a tuple/then a tuple

@doc """
Takes a DateTime and a time zone.

Returns a DateTime for the same point in time, but instead at the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would make this line the first one so it becomes the summary

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also include backticks in DateTime and reference the variable (and some rewording):

Returns a `DateTime` for the same point in time in the given `time_zone`.


Returns a DateTime for the same point in time, but instead at the
time zone provided.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would include ## Examples header and new line after the examples

time_zone,
time_zone_data_module \\ :from_config
) do
in_utc = datetime |> to_unix |> from_unix! |> Map.put(:microsecond, datetime.microsecond)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this dance to_unix |> from_unix!, what do you think about including a to_utc/1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this was a quick way to have something working. It can be optimised in various ways. For instance if the DateTime total offset (UTC + standard) is 0 then it does not have to be changed.

in_utc = datetime |> to_unix |> from_unix! |> Map.put(:microsecond, datetime.microsecond)

gregorian_seconds_utc =
:calendar.datetime_to_gregorian_seconds(in_utc |> NaiveDateTime.to_erl())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would write this either:

gregorian_seconds_utc = :calendar.datetime_to_gregorian_seconds(NaiveDateTime.to_erl(in_utc))

or

gregorian_seconds_utc =
  in_utc
  |> NaiveDateTime.to_erl()
  |> :calendar.datetime_to_gregorian_seconds()


{:gap, [latest_datetime_before, first_datetime_after]}

{:error, _} = error ->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would match here on :time_zone_not_found like you did on shift_zone/3

| {:gap, [TimeZoneDatabase.time_zone_period()]}
| {:error, :time_zone_not_found}

def from_naive(naive_datetime, time_zone, time_zone_data_module \\ :from_config)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe drop the _module suffix and use time_zone_database?

end

def now(time_zone, time_zone_data_module) do
utc_now() |> shift_zone(time_zone, time_zone_data_module)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think single pipes are discouraged, maybe shift_zone(utc_now(), time_zone, time_zone_data_module)?

end

@doc """
Returns the current datetime in the provided time zone.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would reference the variable:

Returns the current datetime in the provided `time_zone`.

try do
time_zone_data_module.by_wall(time_zone, gregorian_seconds)
rescue
UndefinedFunctionError ->
Copy link
Member

@fertapric fertapric Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might move these checks to the function clause time_zone_data_module_from_parameter(:from_config):

defp time_zone_data_module_from_parameter(:from_config) do
  case :elixir_config.safe_get(:time_zone_module, :from_config) do
    time_zone_data_module -> time_zone_module
    nil -> raise "<A good error message>"
  end
end

If a time_zone_data_module is provided, it should implement the TimeZoneDatabase behaviour. If the module does not, I think the error is quite easy to understand: ** (UndefinedFunctionError) function MyTimeZoneDatabase.by_utc/2 is undefined or private.

Beside, I would use the same name for the variable (now time_zone_data_module) and for the key in the configuration (now time_zone_module).

@callback leap_seconds() :: [{:calendar.datetime(), integer()}]

@doc """
Returns a datetime tuple with the UTC datetime for when the leap second
Copy link
Member

@fertapric fertapric Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe clarify that this datetime is not an Elixir's DateTime map/struct saying something like Returns an Erlang datetime tuple?

On the other hand, being this defined in the Elixir world, what are the benefits of returning :calendar.datetime() instead of Calendar.naive_datetime() (or Calendar.datetime())?

@josevalim
Copy link
Member

Since this is an earlier proposal, we should focus on reviewing the code and its features. Once we agree on the API and everything else, we can do a more thorough review on the docs. :)

@lau
Copy link
Contributor Author

lau commented Jul 17, 2018

Thank you for the feedback @michalmuskala

We should probably use the Calendar.iso_days abstraction instead of gregorian seconds in the interfaces - this will keep consistency with other elements of the calendar system and should limit required conversions.

A nice thing about gregorian seconds is that it is a simple integer and it fits well with the time zone data. The tzdata from IANA is not calendar agnostic -- it is in the proleptic Gregorian calendar (which Calendar.ISO implements). Furthermore the level of detail is at the seconds level. So implementing the database and querying can be done for instance by comparing integers in a guard clause in a macro or in an ETS query. Imagine that you have a UNIX timestamp (1531785600) and want to find out the time for that in a certain timezone. All you have to do is: TimeZoneDatabase.by_utc("Europe/Copenhagen", 1531785600 + 62167219200). And the database can use the same format internally.

However, in the examples I have called the functions in the Erlang calendar module directly for converting to and from gregorian seconds. To avoid that we can instead use functions in Calendar.ISO module. There is already a private function for converting iso seconds (ie. gregorian seconds) to a datetime. That could be made public and a reverse could be made: datetime to iso seconds.

This way we avoid adding complexity to the database API and the database implementations.

The :elixir_config module is private - we should probably have some wrapper for it or just use the application env.

Yeah the calling :elixir_config directly part is one of the details that aren't ready yet and was just to have something simple working.

In the data structure returned from the database callbacks there are fields that are not used in the actual implementation. The implementation only uses the values of utc_offset, std_offset, and zone_abbr. I think we should keep the interface minimal and not include the other data, if we don't need it. An option could be also to use tuples instead of maps, but I'm not sure about that one - it might limit us on extensibility in the future.

The other ones are used for when there is a gap. Then it is nice to know what the limits of the gap are. However currently only from_wall and until_wall are used in the current functionality, so from_utc and until_utc could be dropped. I considered make two kinds of periods: one with the limits and one without. (For simplicity in the first draft I stuck with a single period type.) For the by_utc functions the "lighter" periods can be returned. For by_wall - and then maybe only when there is a gap - the "fuller" version that includes the limits can be returned.

Some functions, when information is lacking, return a list of possible values. Is it actually a list or can there always be just two values? If it's just two values, I think it should return something like {:gap, lower, higher} instead.

Yes, there are always two periods if it is ambiguous or there is a gap. Having them directly in the tuple sounds good.

I think, by default, we might provide a database module that has only information for UTC and raises nice errors for other timezones. This should simplify some code paths.

Regarding a mock database that only has UTC, I could see that having some advantages, but we might want to have special shortcuts for UTC anyway even when there is a proper database. And then for the user how would that be explained? "You don't have a database (but internally there really is one anyway)" or "You haven't configured a database, so there is this mock database that doesn't really have any data". I think there are advantages to saying: "error, you have not configured a database". And the concept of a lack of a database being more simple than a mock one. If someone tries to get the time for a timezone when there is no timezone database the error should probably be something like "database missing" instead of "that timezone doesn't exist - well maybe it does, but this is a mock database".

I'm not sure about the leap_seconds callback. It seems to me that the information we want to obtain there is - "What is the difference between TAI and UTC at the specified point in time". If so, I think this function should receive a datetime value (probably in the "iso days" format) and return the integer offset. If we go with that approach, it might return: {:ok, non_neg_integer()} | {:incomplete_data, offset} or something similar, if requested for data too far into the future.

We also want to know for instance "is this a leap second"? "What is the net leap second difference between these two datetimes"? There will be more functions in the API. If we want more or a different functionality in the future then the API will have to change. If we just have all of the leap seconds, then the API won't have to change, but of course it adds more complication to the Elixir (TimeZoneDatabase client) side. It might make sense to implement the features we want now and then see what features are needed and see what would be needed for those in the API. I will do some work on that.

@josevalim
Copy link
Member

A nice thing about gregorian seconds is that it is a simple integer and it fits well with the time zone data. The tzdata from IANA is not calendar agnostic

We can likely still pass iso_days to the database and the database can then convert it to gregorian seconds and perform the lookups. We should not mix the contract with what is the most convenient implementation. :)

In particular, we have removed all the dependencies we had in the :calendar module by our own contract, which is iso_days, so we should not add calls to :calendar back unless it is happening in the time zone database implementation.

@lau
Copy link
Contributor Author

lau commented Jul 20, 2018

I switched from ISO seconds to Erlang style datetime tuples (:calendar.datetime). This means less conversion on the Elixir standard library side because the DateTime and NaiveDateTime structs already have all of the elements of such a datetime tuple.

Non-ISO calendars are now supported in the time zone functions. They have to be compatible (convertible) with the ISO calendar because the time zones are defined in ISO terms.

@josevalim
Copy link
Member

josevalim commented Jul 20, 2018

Thanks @lau!

We worked really hard to no longer depend on Erlang's :calendar library, as that caused a confusion of formats. Sometimes we were passing tuples, sometimes we were passing structs and sometimes we were passing iso days. So I wouldn't like to add dependency on the Erlang formats back.

You have already said that the timezone database would compare integers, as that is the most efficient format, and the iso days already get you half way there without wasting CPU work, so why not rely on iso_days?

To be more precise, iso_days is :calendar.date_to_days and :calendar.time_to_seconds, the amount of work remaining to get the total amount of a datetime in seconds is minimal.

@lau
Copy link
Contributor Author

lau commented Jul 21, 2018

@josevalim

To be more clear about what I wrote the other day:

However, in the examples I have called the functions in the Erlang calendar module directly for converting to and from Gregorian seconds. To avoid that we can instead use functions in Calendar.ISO module.

Ie. we don't have to depend on the Gregorian seconds functions in Erlang's :calendar module even if using ISO seconds. One of the functions already has a pure Elixir implementation in Calendar.ISO (

defp iso_seconds_to_datetime(seconds) do
). I tried using that and added the reverse function: datetime to iso seconds. That worked fine. But then I tried the Erlang style datetime instead of ISO seconds and that was simpler. Just like the ISO seconds solution, the current implementation with datetime tuples does not use any functions from the Erlang :calendar module.

ISO days (with the fraction needed) is not an integer, but rather a nested tuple with different integers. So the TimeZoneDatabase implementation would probably still want to convert it to an integer like ISO seconds. If you provide a DateTime or NaiveDateTime to a function that needs to convert it, you already have the year, month, hour etc. and just need to take those values and put in a tuple.

ISO days seems like a useful solution for converting between calendars, but this is all Gregorian. The time zones are defined in Gregorian terms in the database. All of them would look like this: {_, {_, 86400000000}} and 86400000000 would always have to be the same and that would be sent back and forth billions of times. As I see it, it is an extra step: going from the values such as year, month, ... second, calculating the equivalent ISO days tuple, converting that to whatever the TimeZoneDatabase uses internally (maybe something like ISO seconds).

The Erlang style tuples (e.g. DateTime.utc_now |> NaiveDateTime.to_erl) are simple, easy to read and reason about and the values are already available in the structs passed to the functions. ISO seconds are compact and useful for comparisons.

@josevalim
Copy link
Member

ISO days seems like a useful solution for converting between calendars, but this is all Gregorian.

There is one implementation that is gregorian, we can't assert all possible implementations would like to stay gregorian. ISO days is the format between different parts of Calendar and we need to stick to it here.

As I see it, it is an extra step: going from the values such as year, month, ... second, calculating the equivalent ISO days tuple, converting that to whatever the TimeZoneDatabase uses internally (maybe something like ISO seconds).

If a time zone database uses ISO seconds, this is not an extra step. After all, the first thing you will is to convert the date to ISO days and the time to ISO seconds. Sure, there is a very minor annoyance which is the ISO days tuple with 86400000000 but handling that is literally a 2 LOC function (and the code for it is already in the codebase).

So if you want to keep passing datetime tuples for now, then sure, but at some point before or after merging you or someone in the Elixir team should rewrite it to ISO days+seconds.

At the end of the day, Elixir's calendar code should not have references to the :calendar module beyond the to_erl/from_erl functions. Not even in types, as that increases the surface area: developers now need to learn about Elixir, Erlang and ISO calendar representations.

Thanks!

@lau
Copy link
Contributor Author

lau commented Jul 21, 2018

@josevalim Thank you for your thoughts on it. I enjoy discussing the rationale behind it to exchange ideas on it :) It am not dead set on any particular type.

When it comes to supporting leap seconds, we would want a type that supports leap seconds. For instance you can do ~N[2016-12-31 23:59:60] |> NaiveDateTime.to_erl |> NaiveDateTime.from_erl! and get back to ~N[2016-12-31 23:59:60].

With ISO days I don't get the same thing back:

iex> Calendar.ISO.naive_datetime_to_iso_days(2016, 12, 31, 23, 59, 60, {0, 0}) |> Calendar.ISO.naive_datetime_from_iso_days
{2016, 12, 31, 24, 0, 0, {0, 6}}

Should we use ISO days for the time zones and some kind of tuple or map that supports leap seconds for leap seconds? I also remember talk from a long time ago about a NaiveDateTime struct, but just with the struct part removed or ignored. Basically the Calendar.naive_datetime type.

BTW I saw that :calendar.datetime is used in File.touch/2. Should that also accept a UTC DateTime struct in addition to the tuple? And have the use of a tuple deprecated there?

P.S. @michalmuskala I forgot to mention that I moved the results out of the list for gaps and ambiguous as you suggested. E.g: {:gap, lower, higher}.

@josevalim
Copy link
Member

BTW I saw that :calendar.datetime is used in File.touch/2. Should that also accept a UTC DateTime struct in addition to the tuple? And have the use of a tuple deprecated there?

We will need to do something eventually but that's a separate discussion since :calendar.datetime is part of the contract there and we need to remain backwards compatible (i.e. it was added before we had calendar types).

lau added 10 commits August 9, 2018 16:22
When trying to create a DateTime with the second being 60 (positive leap
second), check to see if that is a leap second known by a
TimeZoneDatabase.
When a TimeZoneDatabase has not been set, an error tuple is desired.
When something else goes wrong, raising makes sense.
If a leap second is outside of the known range of leap seconds,
return it, but not tagged with :ok. It might turn out to be a valid leap
second in the future, but we cannot be sure. The user gets to decide
what to do.
For potential future use in the DateTime module.
And make sure leap seconds are correct in the examples.
There was no leap second in 1971 despite an entry in
the `leap-seconds.list` file distributed by IANA.
@lau
Copy link
Contributor Author

lau commented Aug 10, 2018

@josevalim
The functions/callbacks have been renamed.

Replace with tuples when returning result for a :gap
@josevalim
Copy link
Member

Hi @lau! I will review this for merge. I will drop comments here, for guidance, but you don't have to worry about them, as I will tackle them post merge. If I have any questions I will ping you.

@josevalim
Copy link
Member

I also want to add that, after a IRC convo in July, @lau and I agreed to not build leap seconds or consider leap seconds in the add/diff APIs. That's because if we start building leap seconds and handing off to other APIs, they can flat-out deny leap seconds. So we will follow on the footsteps of JodaTime and parse leap seconds but convert them to the previous second.

@lau
Copy link
Contributor Author

lau commented Oct 26, 2018

I also want to add that, after a IRC convo in July, @lau and I agreed to not build leap seconds or consider leap seconds in the add/diff APIs. That's because if we start building leap seconds and handing off to other APIs, they can flat-out deny leap seconds. So we will follow on the footsteps of JodaTime and parse leap seconds but convert them to the previous second.

Yes, and I would just like to repeat that this ignoring leap seconds was just about the default behaviour standard add/diff functions. So that leap seconds will be ignored for those functions by default. E.g. if you pass a positive leap second to an add function or diff function it will be treated as the second before. And any leap second between the two datetimes will be ignored in the calculation.

Parsing of strings to DateTime/NaiveDateTime/Time structs should still correctly parse the perfectly valid datetime. E.g. DateTime.from_iso8601("2016-12-31T23:59:60Z") should still return
{:ok, #DateTime<2016-12-31 23:59:60Z>, 0} as it does today.

Regarding talking to APIs that are not able to handle valid ISO datetimes if they are leap seconds - this should not be a problem as long as the defaults are used. Most computers today do not create timestamps with leap seconds anyway. Even DateTime.utc_now does not.

Now for something we also discussed adding to the API at some point:
We also talked about a separate function to calculate leap second diffs DateTime.leap_seconds_between which could then be added to the result in order to calculate diffs/adds taking leap seconds into account. In most cases with a small interval this function would return 0. But the absolute result of this if the diff is between two datetimes many years apart e.g. between the years 2014 and 2004 the result would be 3 seconds. For a diff these 3 seconds could simply be added to a result of a diff and the result in seconds would be 3 more. So far so good.

However now that I think about an add operation where we want to take leap seconds into account: Again this is a situation where a user is not using the defaults of an add function, but actually wants to do calculations where leap seconds are taken into account. First we use DateTime.leap_seconds_between and get a number of seconds, then add that to another amount of seconds before passing it to DateTime.add: The add function would have to take the leap seconds into account in order to get the right result in case the result should be a leap second. E.g. 2016-12-31 23:59:60 is a leap second so adding 1 to 2016-12-31 23:59:59 we want a function to return 2016-12-31 23:59:60. So for adding I am not sure that a DateTime.leap_seconds_between function is enough. Some ideas are either a boolean in the add function take_leap_second_into_account \\ false or adding a separate function e.g. DateTime.add_with_leap_seconds function that works just like DateTime.add except it takes leap seconds into account.

def from_naive(%{calendar: calendar} = naive_datetime, time_zone, tz_db_or_config)
when calendar != Calendar.ISO do
# For non-ISO calendars, convert to ISO, create ISO DateTime, and then
# convert to original calendar
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to mention this is not the most efficient nor the most correct way to go about this because it doesn't give the opportunity for databases which cannot be converted to ISO to handle timezones. The correct way would probably to add a new callback to Calendar. However, I am not even sure if there is a calendar that fits this criteria, so I would rather wait until an use case proves this necessary. So I am perfectly happy to ship this as is! 👍 ❤️

end

def shift_zone(%{calendar: calendar} = datetime, time_zone, tz_db_or_config)
when calendar != Calendar.ISO do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since shift_zone relies on iso days conversion, we can completely skip those conversions and generalize the clause above. I have a patch ready.

@josevalim josevalim mentioned this pull request Nov 12, 2018
@josevalim
Copy link
Member

Closing in favor of #8383.

@josevalim josevalim closed this Nov 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants