Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 4 commits into from

Conversation

HenningCode
Copy link

@HenningCode HenningCode commented Apr 28, 2024

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:

  • Add parsing of negative durations (-P1D)
  • Add tests
  • Add documentation for Duration
  • Update all the documentations

@HenningCode HenningCode marked this pull request as draft April 28, 2024 16:47
@hoodie
Copy link
Owner

hoodie commented Apr 28, 2024

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.

src/components/alarm.rs Outdated Show resolved Hide resolved
src/components/alarm.rs Outdated Show resolved Hide resolved
examples/alarm.rs Outdated Show resolved Hide resolved
@hoodie
Copy link
Owner

hoodie commented Apr 29, 2024

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
Copy link
Author

HenningCode commented Apr 29, 2024

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
Copy link
Owner

hoodie commented Apr 29, 2024

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
Copy link
Author

HenningCode commented May 2, 2024

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
Copy link
Owner

hoodie commented May 2, 2024

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants