feat: allow for a configurable time zone database

[benwilson512]

Hey folks!

We're in the middle of trying to minimize our dependency on Hackney, after a variety of recently released CVEs and the decision to only remediate them in ~ 4.0. Unfortunately tzdata has a direct dependency on hackney.

That said we were already on a path to remove tzdata as tz has a more rigorous overall approach, and this one library is the standout on our end.

This PR makes tzdata optional, and allows the user to specify one for this library. If one is not specified it falls back to whatever is configured on Elixir itself.

While I realize the motivation for this PR on our end is pretty tangential to this library's goals, I do think the approach of not binding to a specific timezone database is a good one.

[hopsor]

hi @benwilson512,

First of all, thanks for your contribution. It's great learning that someone is using this package and takes care providing improvements.

I've been thinking about your PR and, taking into account the current maintenance state of tzdata, I was considering directly replacing tzdata with tz. However, as there are multiple choices out there (tz, tzdata, time_zone_info and zoneinfo) that people can be using in their projects, I think it's the best to give them flexibility to use whatever they prefer.

I'm wondering what's the best way of articulating the flexibility you're introducing with this change.

Until now tzdata was a hard dependency and the user of open_hours wasn't required to do anything special. By relying now on Application.get_env(:open_hours, :time_zone_database, Calendar.get_time_zone_database()) we are expecting the users to have set up a timezone database in his application, otherwise open_hours will fail as you can see in the tests. Users having setup a timezone database before hand is the most likely scenario, but not necessarily. So I feel this is a breaking change and we have to properly think about it.

I'm not against introducing breaking changes, actually the opposite. This looks like a good change providing flexibility to the users so it can be a good moment for a first major release, however we need to figure out the best way to do so.

[benwilson512]

An excellent point. While I think it's in practice extremely unlikely this would mess someone up I do think it's reasonable to make this a 0.4 release. This sort of change would be pretty expected in a pretty 1.0 library.

Notably if for whatever reason a user did not want to configure Elixir itself or if they wanted to configure a different database for open hours, that is possible under this PR.

I will update the PR to ensure the tests pass, as well as provide some changelog / readme entries to cover the full config story.

[hopsor]

That'd be awesome @benwilson512

I have just merged a change in the CI that will unblock the elixir 1.18 ci test. It was failing due to a Erlang certificate problem. So please rebase your branch :-)

[benwilson512]

Synced with main and changelog / readme notes added.

[benwilson512]

Ah there we go the CI should pass now.

[benwilson512]

@hopsor any further changes I need to do here?

[hopsor]

hey @benwilson512, thanks for the changes. I'll properly review them during this week

[hopsor]

Hey @benwilson512, first of all apologies for my super late feedback. I've been quite busy lately with different topics and I couldn't have a proper look earlier.

I have left you some feedback. Let me know wdyt

[hopsor]

please remove this change, it'll be automatically updated by release please

[hopsor]

this should be removed as well, CHANGELOG.md is maintained by release please

[hopsor]

I suggest to not call Calendar.get_time_zone_database(). If the OTP app isn't configuring any time zone database it'll return Calendar.UTCOnlyTimeZoneDatabase which I don't think we want. Instead I'd call Application.get_env(:elixir, :time_zone_database).

I'm using in a personal project astro package and it also relies in time zone databases:

https://github.com/kipcole9/astro/blob/main/lib/astro.ex#L106-L121

https://github.com/kipcole9/astro/blob/main/lib/astro.ex#L1612C7-L1612C56

I think what astro is doing is something we could follow as well. I mean the fallback to @compile_time_time_zone_db. That way if the user of the package doesn't have anything explicitly configured then open_hours should still work.

WDYT?

[hopsor]

If we follow my suggestions here I'd say that time zone database priority is:

1. config :open_hours, :time_zone_database
2. config :elixir, :time_zone_database
3. Tz.TimeZoneDatabase
4. Tzdata.TimeZoneDatabase