hoodie / icalendar-rs

📆 icalendar library, in Rust of course - for fun
Apache License 2.0
126 stars 34 forks source link

Initial commit for the introduction of the iCalendar-Duration struct. #94

Closed HenningCode closed 6 months ago

HenningCode commented 6 months ago

Hi @hoodie,

I wanted to update the parser and update the output of the property command. I read into a lot of parsing libraries and all seem to have a pretty lackluster API for creating durations. Thats why I decided to create a wrapper for the ISO8601 duration with the addition of the sign.

I wanted to check with you if this is an approach you would agree with, before I put in to much effort with updating all the documentations.

Missing Tasks:

hoodie commented 6 months ago

Hi, thanks for putting in the work. Looks pretty great so far. I'm wondering if you can maybe get away with even fewer changes. Looks like you are replicating a lot of the work here by redefining every field of your new Duration type. Maybe you could just wrap iso8601::Duration? Just thinking since that type already implements Display.

hoodie commented 6 months ago

by the way I just realized: we could also add those builder methods to iso8601::Duration if you want, I happen to maintain that too ;)

HenningCode commented 6 months ago

Hi, thanks for putting in the work. Looks pretty great so far. I'm wondering if you can maybe get away with even fewer changes. Looks like you are replicating a lot of the work here by redefining every field of your new Duration type. Maybe you could just wrap iso8601::Duration? Just thinking since that type already implements Display.

I thought about it too, but wanted to avoid having to match the attribute, because its an enum. My main gripe with the ISO8601 format is that it forbids the use of weeks and other duration designators at the same (e.g. parsing of P1W1D fails), which I think is allowed from the icalendar spec.

I wanted to add some methods to chain the duration creation and than it would break if I combine weeks and days. (e.g. Duration::weeks(1).and_day(1)

hoodie commented 6 months ago

About those weeks and months, here's another early idea: Maybe we should be accepting the Week/Year notation when parsing (in order to be compatible with non-compliant implementations). This is a bit of a stretch, but it is said to more lenient on input and more strict on output. So maybe we should accept just ordniary Durations (including negative ones) and internally convert them to the stricter notation. Just an idea, I can imagine this approach might have some pittfalls here and there.

HenningCode commented 6 months ago

I don't think that's going to work, because year and month are relative to the starting date. So it's actually not possible to calculate the duration of days without information about the event date. Google Calendar, for example, only accepts a maximum week value of four.

hoodie commented 6 months ago

I don't think that's going to work, because year and month are relative to the starting date. So it's actually not possible to calculate the duration of days without information about the event date. Google Calendar, for example, only accepts a maximum week value of four.

that sounds fair

HenningCode commented 6 months ago

I am closing this PR, because I created a PATCH for the icalendar_duration package vlarmd. If it gets accepted than it would be fairly easy to implement that for this package. I already did implement it, but not test it thoroughly. The branch is called feature/implement-icalendar-duration, if you want to have a look. I will create an PR if the changes on icalendar_duration get accepted.