Time zone handling: convert DateTime to UTC, NaiveDateTime to floating reference

strohel commented 5 years ago

This introduces proper time zone and "floating" date-time handling for Component::start() and Component::end(), by following means:

we introduce new thin enum wrapper CalendarDateTime, which is either floating (as per RFC) or in UTC (extendable in future for timezone-referenced date-time).
we make Component::start(), Component::end() accept our CalendarDateTime instead of chrono::DateTime<TZ> (more precisely we accept T: Into<CalendarDateTime>).
we implement From<chrono:DateTime<TZ>> for CalendarDateTime, restoring source compatibility with previous versions (but not interpretation compatibility), and providing caller convenience. The implementation converts passed date-time to Utc.
we also implement From<chrono:NaiveDateTime> for CalendarDateTime, which converts to floating date-time per the RFC. This allows users to opt-in for this special behaviour.

As demonstrated by the test, this is a change of behaviour of existing user code (and should be probably mentioned in docs or a kind of changelog):

If user pased date-time "2014-07-08 09:10:11 CEST (+02:00)", the library would previously emit "20140708T091011" (floating), now it emits "20140708T071011Z" (Utc). I argue that the current behaviour is more or less incorrect, as chrono DateTime conveys mandatory time-zone information, which currently gets discarded.

This change allows users to explicicly opt-in to the "flating" behaviour by passing NaiveDateTime. This shuold however not be frequent, even the RFC mentions:

In most cases, a fixed time is desired.

Note that some some eligible date-time fields are not changed by this commit: Todo::due() and Todo::completed().

Fixes #2.

strohel commented 5 years ago

Hi @hoodie, consider this as a request-for-comments MR, all the implementation details including naming and placement of the new type (or a different approach) being up for discussion.

hoodie commented 5 years ago

@strohel you don't happen to be at rustfest?

strohel commented 5 years ago

@strohel you don't happen to be at rustfest?

Unfortunately I'm not, I wish I would be! :)

hoodie commented 5 years ago

ok, I'll try to look at this some time this week, thanks for the help already :D

hoodie commented 4 years ago

So I finally got around to looking at this. Do I understand it correctly that the TimeZone is not actually stored inside the DTSTART/DTEND property?

strohel commented 4 years ago

Do I understand it correctly that the TimeZone is not actually stored inside the DTSTART/DTEND property?

If I understand well that you refer to RFC 5545 (Section 3.3.5), then the answer is No, not in general:

One can store a) special "floating" date-time b), UTC date-time, c) date-time with reference to a time zone previously defined in VTIMEZONE item.

Per https://stackoverflow.com/a/42941087 the c) has been added mainly because of correct handling of recurrent events across daylight saving time timezones. For other use-cases, b) (and a) in special cases) should be sufficient.

This PR adds support for b) (in addition to current "abused" a)), while having the door somewhat open for c). However it seems that for c), lone chrono::DateTime<TZ> would not suffice as chrono only supports "fixed offset timezones". One would need to reach for https://github.com/chronotope/chrono-tz/

The details of the API and implementation should be discussed, for example how strict we shall be now: accept strictly Utc date-times (in addition to naive date-times), or silently convert to Utc?

hoodie commented 4 years ago

I the spirit of being rusty I'd like to not implicitly convert. I think I remember seeing this before and though that the requirement to add fully written out time zones is a bit of a pain. I really like to progress here. Your PR is a great step in the right direction already. Have you an idea for a nice API to let the user decide whether to convert to UTC or add the actual timezone?

strohel commented 4 years ago

I the spirit of being rusty I'd like to not implicitly convert.

Got it, makes good sense.

I think I remember seeing this before and though that the requirement to add fully written out time zones is a bit of a pain.

Yes it seems so. It seems that it stemmed from the fact that TZ definitions and their identifiers were not harmonized across systems at the time, so theŕe is a mechanism to embed them. A TZ definition seems more complicated than my initial thought of just a stored offset, see e.g. https://data.iana.org/time-zones/releases/tzdata2019c.tar.gz

Have you an idea for a nice API to let the user decide whether to convert to UTC or add the actual timezone?

Hopefully so.

It would be technically possible to let users embed VTIMEZONE items manually and then manually specify TZIDs when setting date-times, but that seems error-prone and unergonomic. So a sort of automatic collection and serialization of used timezones would be better. From a quick research, the existing chrono::offset::TimeZone trait does not expose enough information to generate VTIMEZONE. In general adding support for VTIMEZONEs and TZIDs would be non-trivial (but certainly doable).

OTOH, o lot of use-cases would be hopefully achievable by just allowing users to decide between floating date-times and Utc date-times (that is the case with my downstream crate). So an iterative approach would be viable here.

What about accepting following enum in the date-time fields (the same as is in the PR diff, shortened):

#[derive(Clone, Copy, Debug)]
pub enum CalendarDateTime {
    /// FORM #1: DATE WITH LOCAL TIME: floating, follows current time-zone of the attendee.
    Floating(NaiveDateTime),
    /// FORM #2: DATE WITH UTC TIME: rendered with Z suffix character.
    Utc(DateTime<Utc>),
    // Here it would be possible to introduce FORM #3: DATE WITH LOCAL TIME AND TIME ZONE in the future
}

A breaking API change, but possibly is is positive as it would warn users of changed interpretation.

At this point it would be possible to stop and release that for now. But it would mean users would have to call

let chrono_dt: DateTime<Utc> = ...
event.starts(CalendarDateTime::Utc(chrono_dt));

For added ergonomics i suggest we accept T: Into<CalendarDateTime> instead (as it is the case in current MR code), but provide only: From<NaiveDateTime> -> CalendarDateTime::Floating and From<DateTime<Utc>> -> CalendarDateTime::Utc for CalendarDateTime.

This would be also an API breaking change for all calls but the ones that already use Utc as TZ generic argument.

In the future we could add the third CalendarDateTime variant TimeZoneReferenced(...) + provide From<DateTime<TZ: (hypothetical) TimeZoneInfo>> -> CalendarDateTime::TimeZoneReferenced -- though I'm not sure whether this would be possible (with our without breaking the API) given Rust specialization disambiguation rules.

What do you think? (sorry for such a long post)

hoodie commented 4 years ago

"sorry for the long post"? this was great, I was a bit out of scope after not thinking about this for over a year. I like it, thank you so much 👍

strohel commented 4 years ago

Cool, I'll update the PR in coming days to match the suggestion, that will be just a small change.

I'll also probably convent the rest of the places working with datetime, Todo::due and ::completed.

Where do you think a good place for a changelog/announcement of these changes would be? Main page of the documentation?

hoodie commented 4 years ago

I still haven't found a good changelog tool

strohel commented 4 years ago

@hoodie MR changed, finished, shall be ready for review.

strohel commented 4 years ago

Friendly ping, will you have a change to look at the diff? It is +116 −34, but most added lines are docs/tests.

hoodie commented 4 years ago

Looks good to me, thank you so much for your contribution. I'll try to make a release tonight including a changelog. Have you by any chance found a nice changelog tool so far?

strohel commented 4 years ago

Cool, thanks!

Have you by any chance found a nice changelog tool so far?

Not really. But I've included the "changelog" in the main page of documented generation. (i.e. it will appear on docs.rs, and that can be probably copy&pasterd to GitHub release notes).

hoodie commented 4 years ago

I'll keep looking for something

hoodie commented 4 years ago

btw: just released 0.7, hope everything is ok, I think I'll go now and get some sun on my skin before it's too dark outside :grinning:

strohel commented 4 years ago

btw: just released 0.7, hope everything is ok, I think I'll go now and get some sun on my skin before it's too dark outside grinning

Thanks a bunch. A do see 0.7.1 on crates.io, but not on https://docs.rs/icalendar/ neither here in GitHub releases (tags)?

hoodie commented 4 years ago

I didn't tag it yet, and why it doesn't show up on docs.rs 🤷‍♂ I don't believe there is anything in there that keeps it from being built on docs.rs

hoodie commented 4 years ago

https://twitter.com/rustlang/status/1198691339984093185 its there now

hoodie / icalendar-rs

Time zone handling: convert DateTime to UTC, NaiveDateTime to floating reference #10