Support for reducing the use of time.Local by allowing overrides.

[arran4]

Attempts to resolve: #99

@brackendawson please review.

Also let me know what comment style is best, happy for you to create a new PR with these commits and your own addendums / fixes of comments.

[arran4]

Updated.

[brackendawson]

This is a breaking change. Anyone who uses types embedding ComponentBase through an interface will have to update the method signature in their interface. If any module has two or more dependencies which do this and they don't all have the same interface, then it will be impossible to build that module.

[brackendawson]

I'd be quite happy adding a new constructor function like this if we want to use the functional options pattern to resolve this.

type Option func(*Calendar)

func ParseCalendarOpts(r io.Reader, opts ...Option) (*Calendar, error)

[arran4]

The constructor is unlikely to be in any interface as it doesn't have a receiver.

Should I provide a proxy struct for quick adaptation. (This library is still pre v1 which is not something I want to say but there are a couple of changes I might need to make that are breaking. If only there was a way of finding out who was using what...)

type NoOpComponentBase struct {
   *ComponentBase
}

func (nocb *NoOpCompontentBase) GetEndAt() (time.Time, error) {
  return nocb.ComponentBase.GetEndAt()
}

Which could be used as a dropin replacement? + Documentation. A little more work when they upgrade but hopefully something I could put in the function documentation.

Does that work? Or will I need to:

func (cb *ComponentBase) GetEndAt() (time.Time, error) {
}

func (cb *ComponentBase) GetEndAtWithOptions(ops ...any) (time.Time, error) {
}

?

Are we talking specifically about GetEndAt or the constructor?

---

Also what is the goal of using func() for the optional arguments?

[brackendawson]

You are right, you can make breaking changes without breaking the modules promise as you're v0.x. But you are easily the most popular (used by 327 public repos) and most mature Go ICS library. If it were my inbox I'd avoid breaking changes where practical. Otherwise jumping right to v2 to make breaking changes would be better.

I'm not sure how you would wire NoOpComponentBase in? As ComponentBase is exported, changing the type that things like VAlarm embed is also a breaking change.

I still think choosing a fallback location when the Calendar is instantiated is pretty clean, and a new constructor a pretty clean compatible change to do that. That's my opinion.

The reason for making option a type is so its clear what you provide, in my example it would use:

// InLocation provides an Option to set the fallback location. time.Time values
// for events in the calendar which have no explicit location, such as all day
// events, will use this location. The default fallback location is time.Local.
func InLocation(l time.Location) Option {
    return func(c *Calendar) {
        c.fallbackLocation = l
    }
}

So that you can write:

cal, err := ics.ParseCalendarWithOpts(r, ics.InLocation(userLocation))

and according to Rob Pike interface{} says nothing.

[arran4]

I've replied in PR:
#107 (comment)

[arran4]

Conflict resolving time!

I have responded, I suspect the inline response was supposed to be more general?

[arran4]

Conflicts resolved.

[arran4]

Hi @brackendawson I'm moving this out of the thread and into the PR; this is response to: #107 (comment)

Otherwise jumping right to v2 to make breaking changes would be better.

Probably a v0.2 then?

I'm really at a loss as to what to do here. I let some API conventions in which I kind of regret.

I feel that a API breakage with a minor change to fix is preferable to a behavior change. However API pollution is preferable to both.

cal, err := ics.ParseCalendarWithOpts(r, ics.InLocation(userLocation))

I hadn't actually considered that... I think using .Local was definitely very wrong, but having a "file wide" solution is too as the way ical is implemented is that it allows for multiple timezones; see:

https://github.com/arran4/golang-ical/blob/master/cmd/issues/test97_1/main.go#L24-L25

While this definitely works for single timezone files. (Although if they haven't specified tz..) I also think the code base needs better handling of timezones, especially custom ones as in the linked example. Having a look at SetDuration and SetStartAt etc, they definitely miss that use case.

Even completely avoid it here:

func (cb *ComponentBase) SetStartAt(t time.Time, params ...PropertyParameter) {
	cb.SetProperty(ComponentPropertyDtStart, t.UTC().Format(icalTimestampFormatUtc), params...)
}

Just assumes UTC is fine -- In most cases that will work, however RRULE that's going to be an issue.

(I would love to spend the time to really dig into this, and release a v1+ with all these things considered.)

The proposal about NoOpComponentBase was to provide it was a drop in previous public interface compatible wrapper, so people that are exposing it, etc, can wrap their Type. I would probably have to produce one for VAlarm, VEvent, etc, yes. I think only a minority of people would be effected, and I should definitely add a function comment. But also immediately "depreciate" them. I'm happy to do that if it means it unstucks this.

So I guess;

1. Multiple Timezone files, how?
2. NoOpVEvent, NoOpVAlarm, I'm not entirely expecting these to be acceptable but I feel we are working the problem.

[brackendawson]

Probably a v0.2 then?

If you choose to introduce a breaking change then it would be most helpful to move directly to v2.0, this is because your module name would change to github.com/arran4/golang-ical/v2. This allows people to import v0 and v2 in the same program and avoid the situation where dependencies importing different APIs of golang-ical would break a build.

I'm really at a loss as to what to do here. I let some API conventions in which I kind of regret.

I feel that a API breakage with a minor change to fix is preferable to a behavior change. However API pollution is preferable to both.

Oh believe me I know, even the Go maintainers bump into this, there's more than one place in the standard library where the API has had to be extended.

cal, err := ics.ParseCalendarWithOpts(r, ics.InLocation(userLocation))

I hadn't actually considered that... I think using .Local was definitely very wrong, but having a "file wide" solution is too as the way ical is implemented is that it allows for multiple timezones; see:

https://github.com/arran4/golang-ical/blob/master/cmd/issues/test97_1/main.go#L24-L25

While this definitely works for single timezone files. (Although if they haven't specified tz..) I also think the code base needs better handling of timezones, especially custom ones as in the linked example. Having a look at SetDuration and SetStartAt etc, they definitely miss that use case.

Even completely avoid it here:

func (cb *ComponentBase) SetStartAt(t time.Time, params ...PropertyParameter) {
	cb.SetProperty(ComponentPropertyDtStart, t.UTC().Format(icalTimestampFormatUtc), params...)
}

Just assumes UTC is fine -- In most cases that will work, however RRULE that's going to be an issue.

(I would love to spend the time to really dig into this, and release a v1+ with all these things considered.)

The proposal about NoOpComponentBase was to provide it was a drop in previous public interface compatible wrapper, so people that are exposing it, etc, can wrap their Type. I would probably have to produce one for VAlarm, VEvent, etc, yes. I think only a minority of people would be effected, and I should definitely add a function comment. But also immediately "depreciate" them. I'm happy to do that if it means it unstucks this.

So I guess;

1. Multiple Timezone files, how?

2. NoOpVEvent, NoOpVAlarm, I'm not entirely expecting these to be acceptable but I feel we are working the problem.

So the problem we were trying to address is how to convert ICS timestamps which have no known time zone into Go time.Time instances. A fallback time zone is absolutely the right thing to do. For a CLI program that someone runs on their machine time.Local is probably the right choice. For a web server handling calendars for multiple users it's definitely wrong, I ended up needing to do this hack. I think in the latter example the fallback time zone needs be be scoped to each ics.Calendar and the programmer needs to be able to set it when parsing or creating a calendar. This is why I proposed a ParseCalendarOpts or similar, I think it solves the issue with minimal pollution to the API.

In the other direction, writing properties:

vEvent.AddProperty(ics.ComponentPropertyDtStart, "20240929T144500", ics.WithTZID("Europe/Berlin"))

I don't think an int timestamp and functional option to set the string time zone is very helpful, especially as parsing the time zone can fail! Better to accept a time.Time which already has both of these properties:

berlin, err := time.LoadLocation("Europe/Berlin")
if err != nil {
    return err
}
vEvent.SetDtStart(time.Date(2024, 9, 29, 14, 45, 0, 0, berlin))

Now there is no error to handle in our code.