arran4 / golang-ical

A ICS / ICal parser and serialiser for Golang.
Apache License 2.0
279 stars 72 forks source link

add SetDuration for VEvent #47

Closed fty4 closed 2 years ago

fty4 commented 2 years ago

Description Add function to add a event duration after start or end was already set.

This helps to simple add a time.Duration value to the event instead of manually calculating the event end time.

New implementation This implementation will check if a start time is already set and will add the duration to this time. If the start time is not present it will try to check if the end time is instead set. When this is the case the duration will be subtracted from the end time.

Problems If no time is set yet (no SetStartAt or SetEndAt called) nothing will happen. The time will be required before - therefore the order is important.

Another implementation would be to deviate from the standart which maybe require a workaround. In my opinion it should not be that big problem because usually you would try first to set start time and add the duration afterwards.

Another problem is that this implementation does NOT use the RFC duration - instead it will calculate the start/end time. When implementing a duration (e.g. DURATION:PT1H30M) and start and end are both set it will fail to import the calendar on MacOS Calendar (not sure if it is allowed by RFC or just not work on Mac). It is only possible to use DTSTART and DURATION at the same time here.

examples:

DTSTART, DTEND and DURATION (fail) ``` BEGIN:VCALENDAR VERSION:2.0 PRODID:-//arran4//Golang ICS Library BEGIN:VEVENT UID:1111 SUMMARY:Test DTSTART:20211208T203000Z DTEND:20211208T220000Z DURATION:PT1H30M END:VEVENT END:VCALENDAR ```
DTSTART and DURATION (success) ``` BEGIN:VCALENDAR VERSION:2.0 PRODID:-//arran4//Golang ICS Library BEGIN:VEVENT UID:1111 SUMMARY:Test DTSTART:20211208T203000Z DURATION:PT1H30M END:VEVENT END:VCALENDAR ```

Therefore it might be better to use this implementation instead of using the PropertyDuration.

It would be great to discuss this issue in this PR or a separate issue.

Example

c := ics.NewCalendar()

duration := time.Duration(float64(time.Hour) * 2)

start := time.Now()

// old way
e1 := c.AddEvent("1111")
e1.SetSummary("old way")
e1.SetStartAt(start)
e1.SetEndAt(start.Add(duration))

// new way - start
e2 := c.AddEvent("2222")
e2.SetSummary("new way start")
e2.SetStartAt(start)
e2.SetDuration(duration)

// new way - end
e3 := c.AddEvent("3333")
e3.SetSummary("new way end")
e3.SetEndAt(start)
e3.SetDuration(duration)

// print
c_str := c.Serialize()
fmt.Println(c_str)

output:

BEGIN:VCALENDAR
VERSION:2.0
PRODID:-//arran4//Golang ICS Library
BEGIN:VEVENT
UID:1111
SUMMARY:old way
DTSTART:20211208T194700Z
DTEND:20211208T214700Z
END:VEVENT
BEGIN:VEVENT
UID:2222
SUMMARY:new way start
DTSTART:20211208T194700Z
DTEND:20211208T214700Z
END:VEVENT
BEGIN:VEVENT
UID:3333
SUMMARY:new way end
DTEND:20211208T194700Z
DTSTART:20211208T174700Z
END:VEVENT
END:VCALENDAR
arran4 commented 2 years ago

Thanks again @fty4 . However since this is different to all the other helper functions can we get a small amount of function documentation on this. Probably just some of what you have in the PR. As people are expecting mostly pass-through functions here.

I will (when / if) I get more time split out the smarter functions from the pass-through. But this is fine where it is right now.

Other than the comment (which wasn't a requirement before TBH) LGTM

arran4 commented 2 years ago

(Saying that icing would be adding a unit test or two. :P )

fty4 commented 2 years ago

First of all thank you for sharing your library - that helped me a lot :)

I didn't add comments because all other functions also had them missing..

I added commits to fulfil your requests:

  1. Adding comment direct on function signature
  2. Adding function test (added components_test.go file)
  3. Adding return value of type error for func SetDuration

I hope the changes are as you requested.

arran4 commented 2 years ago

I didn't add comments because all other functions also had them missing..

Totally cool.

I hope the changes are as you requested.

Thanks. Approved and merged. Sorry it took so long it's been a bit busy.