arran4 / golang-ical

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

Modify component methods to allow chaining? #62

Closed endigma closed 1 month ago

endigma commented 1 year ago

If you write your component methods like:

func (event *VEvent) SetSummary(s string, props ...PropertyParameter) *VEvent {
    event.SetProperty(ComponentPropertySummary, ToText(s), props...)
        return event
}

instead of

func (event *VEvent) SetSummary(s string, props ...PropertyParameter) {
    event.SetProperty(ComponentPropertySummary, ToText(s), props...)
}

The API can be used like:

event := cal.AddEvent("id@domain").
    SetDtStampTime(time.Now()).
    SetStartAt(time.Now().Add(1 * time.Hour)).
    SetEndAt(time.Now().Add(2 * time.Hour)).
    SetSummary("Meeting with Jim")

which reads a little cleaner than

event := cal.AddEvent("L6QlmeERtwbOTfb_Xu9H2@hussle.work")
event.SetDtStampTime(time.Now())
event.SetStartAt(time.Now().Add(1 * time.Hour))
event.SetEndAt(time.Now().Add(2 * time.Hour))
event.SetSummary("Meeting with Jim")

in my opinion

this is also not a breaking change, the API would still work the 2nd way too

arran4 commented 1 year ago

I sort of see this as the builder pattern. The problem with using the builder pattern in go is either you have to create a whole new set of objects, or you can't return error (or you have to abuse panic() and recover()). I don't have too much of an issue with, although personally I find the builder pattern harder to read because I need to scan the line for the . at the end, rather than a lined up event. but that wasn't the reason for it's usage here.

image

Which is already being used that way, so it would be inconsistent:

https://github.com/arran4/golang-ical/blob/fd89fefb01820d2c8b163f368ed475411b0be63e/components.go#L145

Plus I've been using that convention to mean "new": https://github.com/arran4/golang-ical/blob/109346913e546be9f71334e3dad75e9dbbb6db84/calendar.go#L408-L412

I'm not against a PR for either:

I can't guarantee I will merge it, but I won't close it and provide feedback until it's acceptable.

I really think that for this type of pattern to work in go you would need something like: the coalesce operators ?. from Javascript/typescript world where if the 2nd param is an error or the output is nil, handle it.

arran4 commented 1 month ago

Sorry, I'm going to close this for now. It might be possible but probably not in this way. It will likely be in a builder pattern style however I'm not sure it helps people enough.

Feel free to reopen to discuss more, or create a discussion. (What I feel is the most unused) I figure we should design this in a way it doesn't impact the existing public API.