lpil / icalendar

🗓️ A small library for reading and writing ICalendar files.
MIT License
103 stars 56 forks source link

Datetime and Timezone support #7

Closed johnhamelink closed 8 years ago

johnhamelink commented 8 years ago

This PR replaces the erl tuple-based syntax for using Timex's DateTimes instead. This allows us to support Elixir DateTime's, but to also fully support Timezones. In order to keep things simple to begin with, the change currently formats everything to UTC - so the time is accurate but the original timezone is lost.

Later, the TZID attribute can be supported to enable timezone serialization (and deserialization), which would allow us to remove the UTC normalization section of the code.

I'm looking for feedback on this - is it acceptable to drop support for Erlang tuples if we add Timex as a dependency?

lpil commented 8 years ago

I'm unsure about the Timex dep, and also unfamiliar with the library. Am I right in thinking that it provides the functionality to normalize dates to UTC so a user can pass a date type with timezone information in, rather than having to use UTC?

It seems that we could keep the tuple format (avoiding a breaking change) and have that protocol implementation naively convert it to the correct format with the assumption that it is either UTC or locale time (one to be decided). This would open more of a window to user error though, as we're not forcing the user to consider timezones. What are your thoughts here?

lpil commented 8 years ago

The README will need to be updated too if we remove tuple support.

johnhamelink commented 8 years ago

@lpil good questions :smile:

Am I right in thinking that it provides the functionality to normalize dates to UTC so a user can pass a date type with timezone information in, rather than having to use UTC?

For now, yes, but the point of adding it is that in future PRs I can add the ability to add the timezone using tzid (see here) into the ical string - that would be the "correct" way to do it, as it is used in calendars to see if people are likely to be asleep in their native country, etc.

Using Timex also has a second benefit - it gives us a path to using Elixir's native DateTime format once it supports timezones out of the box (see here).

It seems that we could keep the tuple format (avoiding a breaking change) and have that protocol implementation naively convert it to the correct format with the assumption that it is either UTC or locale time (one to be decided).

We could add the native tuple format back in and use the macro you created to detect it, then convert it into a UTC Timex.DateTime and feed it back into the code I've written - that would give us the compatibility you ask for as well as one main code path which is future proof.

This would open more of a window to user error though, as we're not forcing the user to consider timezones. What are your thoughts here?

You could extend the requirements for the tuple to include a string representation of the timezone: anything that's in the IANA Timezone database would be valid. Timex provides that list inside Timex.timezones.

lpil commented 8 years ago

OK, this seems good. Let's put the time tuple support back in and update the README to display both formats being used.

johnhamelink commented 8 years ago

I've added Erlang Tuple interop back in, and I've updated the README

lpil commented 8 years ago

This looks good to me. Is there anything else you'd like to do, or shall I merge?

johnhamelink commented 8 years ago

@lpil this should be ready now, but I'd like to have the other PR merged first so I can rebase and make this PR fit the new HEAD.

lpil commented 8 years ago

Cool. I've merged the other.

johnhamelink commented 8 years ago

@lpil I've just modified the deserialization so it now uses Timex directly to parse the DateTime string, and I've updated the tests so that Events are expected to have DateTimes in them.

johnhamelink commented 8 years ago

@lpil should be ready to merge now :smile:

lpil commented 8 years ago

Thanks!