Clean up date/time handling and add support for date-times with a TZID

hoodie / icalendar-rs

📆 icalendar library, in Rust of course - for fun

Apache License 2.0

126 stars 34 forks source link

Clean up date/time handling and add support for date-times with a TZID #40

Closed qwandor closed 2 years ago

qwandor commented 2 years ago

This fixes some issues and adds some new functionality. In particular:

start_date and end_date were taking a date with a timezone, but then ignoring the timezone. I've removed them, and instead changed starts and ends to take impl Into<DatePerhapsTime>, which includes the NaiveDate case.
Similarly, I've changed all_day to take a NaiveDate rather than a date with a timezone which is then ignored.
According to RFC5545, DTSTAMP must be in UTC, but the timestamp method was accepting a impl Into<CalendarDateTime>. I've fixed this to only allow DateTime<Utc> to match the spec, and similarly fixed the getter to return the same.
I've added support for setting and getting date-time values with a TZID. For now the TZID is just a string, so the client needs to handle VTIMEZONE components manually. I'm not sure if there's a better solution to this, I can't think of one without major changes to the API.

hoodie commented 2 years ago

one more thing on "conventional commits", since you're making a bunch of breaking changes. It would be nice to use the BREAKING CHANGE footer. Those will show up in the changelog as well.

hoodie commented 2 years ago

regarding this

I've added support for setting and getting date-time values with a TZID. For now the TZID is just a string, so the client needs to handle VTIMEZONE components manually. I'm not sure if there's a better solution to this, I can't think of one without major changes to the API.

I was thinking that the method that sets the date-time just takes the TZ and adds the tz component itself

qwandor commented 2 years ago

You mean the starts and ends methods would add the VTIMEZONE to the calendar? The trouble is that currently they are just methods on the Event, so they don't have a way to add another component to the calendar, so you'd need to change the API somehow to make that possible. But I can't think of a nice way to do that, it would give up on the nice separation you currently have where you can construct individual events and then combine them together into a calendar. You'd have to always deal with the calendar as a whole. And then the parsing side is even worse, as the VTIMEZONE might not map to any of the timezones that chrono-tz knows about, so I guess we'd have to create a new timezone type.

qwandor commented 2 years ago

I've added BREAKING CHANGE footers, though they mostly just repeat the rest of the commit message. That's why I just went with the ! syntax originally.

hoodie commented 2 years ago

anything you want to add before I merge this?

qwandor commented 2 years ago

I'm happy as it is. We probably should add a type for VTIMEZONE at some point, but that can be a separate PR, and may require some rethinking of what the Component trait includes.

hoodie commented 2 years ago

Sounds good, maybe that doesn't even have to be a breaking change.

hoodie commented 2 years ago

there we go https://docs.rs/icalendar/0.13.0/icalendar/

thanks for the contribution