Timezone support

[josevalim]

Continuation of #7914.

It removes leap seconds support, so we can have a more fine grained discussion on the topic. It also streamlines the shift_zone operation.

We still need to discuss the API for the timezone client.

[josevalim]

This should be ready for one last review. @lau, can you please take a final look?

Note that instead of :from_config and returning {:error, :no_time_zone_database}, we now ship with Calendar.UTCOnlyTimeZoneDatabase which returns {:error, :utc_only_time_zone_database} for other time zones. I have also namespaced the TimeZoneDatabase module inside Calendar and we now ship with a built-in (but limited) calendar.

Please approve or reject the PR accordingly so we can go ahead and merge it.

[josevalim]

Also, should it be called Calender.get_time_zone_database or simply Calendar.time_zone_database?

[michalmuskala]

Do we really want to duplicate the documentation text like that? I'm pretty sure they will get out of sync pretty quickly. I would forward from one to another - I'm not sure which way, though.

[josevalim]

@michalmuskala yeah. I am ok with them getting out of sync though, I think eventually we will contextualize them better... I am just not sure how people will look for this information, so I want to err on the size of over explaining.

[michalmuskala]

Ok, that's fair. Should we add some comment around the docs that you should change the other place as well?

[michalmuskala]

Should we have an accompanying force_zone function that uses the same naive dt, but in a specific zone? For example, if you got time in the wrong zone, but the numeric values are correct. From experience, I know such a function is useful.

If we don't want to provide such a function, maybe in the docs for shift_zone we describe how to do it by first converting to native and then from naive to the target zone.

[josevalim]

I am not sure what you mean here, can you please provide an example? Or an sketch of how it would look like? Or open up a separate issue. There are other features we likely want to look at, such as finally adding DateTime.add/3 but this PR is already big as is. :)

[michalmuskala]

Sorry for not being clear. The use case I have in mind would be exactly equivalent to something like:

def force_zone(datetime, time_zone) do
  from_naive(to_naive(datetime), time_zone)
end

This means basically that if I have a 9 AM CET and I want to have 9 AM UTC or similar. It's generally useful in many cases, where you actually don't want to do proper time zone conversion.

[lau]

@michalmuskala If you have a DateTime and want the same "wall time" in a different time zone you can pass it to a function only expecting a naive datetime. E.g. 2018-11-13 09:00:00 Europe/Copenhagen and want 9:00:00 for the same date but in UTC instead.

DateTime.from_naive(cph_dt_9oclock, "Etc/UTC") should return the same "wall time" as the one provided, but in UTC instead. Because from_naive just looks at the fields required by the Calendar.naive_datetime type and ignores the time zone part.

[michalmuskala]

I would suggest that we use a record or a raw tuple here. I know it's much less legible, but it's more performant and in things like this we probably want to squeeze every last bit of performance.

[josevalim]

I am not worried about legibility but rather about extensibility. Using a tuple would be very unforgiving in case we need to add new information. A record is a bit better but adding custom fields would be tricky. Btw, if we have many maps, all with the same keys, do they share the keys structure when loading it in memory?

[michalmuskala]

It depends on how it is loaded. In general, though, if we're loading it from ETS, the table will always copy everything. ETS does not do literal sharing.

[josevalim]

I was wondering about modules. But keep in mind we can always store it in ets as a tuple and then convert it to a map when reading out. :)

[fertapric]

I would go with Calendar.get_time_zone_database because it looks more balanced with Calendar.put_time_zone_database, but either way would work for me.

[michalmuskala]

@josevalim I guess it's pretty common to have a /0 function for "get" and a /1 function for "put" with exactly the same name. We could use this convention here. What do you think?

[fertapric]

@michalmuskala do we have examples of that convention in the codebase?

I found examples of get_value() and put_value(value) in Application.get_env/put_env, Module.get_attribute/put_attribute, and System.get_env/put_env.

I also found examples of value() and put_value(value) in Registry.meta/put_meta.

[josevalim]

We also have examples of value() and put_value(value) in Registry.meta/put_meta.

I believe this is the convention @michalmuskala suggestion. :) I am honestly torn on which one to pick. I can say though the ones in Application/System are usually more meta (it is a general key-value) and the ones in Registry is more about setting something, which is more inline with what we have here.

[lexmag]

We have Logger.metadata/0,1 that does what @michalmuskala said.
However, I always wished it have get and put for enhanced readability.

[josevalim]

I went with Calendar.time_zone_database and Calendar.put_time_zone_database per the rationale above.

[michalmuskala]

@fertapric one place where this is used is URI.default_port/1,2

[josevalim]

Squashed and merged manually, thanks everyone!
❤️ 💚 💙 💛 💜

[lau]

This test module is has async set to true for tests. I assume this and Calendar.put_time_zone_database can cause race conditions. Should the tests that call Calendar.put_time_zone_database be put in a separate test module? Or just make the existing module async: false.

cc @josevalim

[josevalim]

Good catch. Let's make remove the async: true from this particular module. I was planning to break this test file apart, so I will do that and fix the async bit. :)

[josevalim]

Pushed!