Defaulting to time.Local is a dangerous assumption

[brackendawson]

ComponentBase.getTimeProp will return times in the time.Local location when time zone information is not present in the ics. This may be fine for a simple program parsing a calendar. But when a HTTP handler parses calendars on behalf of clients this will almost certainly be wrong and break time comparisons in subtle ways. Rather the client's time zone should be used, if available. Failing that it shoud be clearer that some assumption may need to be made by the developer writing the handler.

I propose adding a new constructor similar to time.ParseInLocation:

func ParseCalendarInLocation(r io.Reader, time.Location) (*Calendar, error)

The location will be stored as an unexported variable in Calendar and used for all assuptions.

ParseCalendar shall then simply return this:

func ParseCalendar(r io.Reader) (*Calendar, error) {
    return ParseCalendarInLocation(r, time.Local)
}

[arran4]

@brackendawson I'm happy to accept a PR for the new constructor. I'm a little bit apprehensive about changing or removing the existing one, but perhaps making it vararg so that people could put in an option to change the location.

And/Or a function comment explaining this (I have been meaning to add more and RFC details.)

[brackendawson]

There would be no observable change to the existing constructor.

I like the functional design parameter pattern, but I worry that it would hide itself too well. I'd like to make it clear to developers that the default ParseCalendar has to assume a location. Having a second constructor immediately under it with an additional required argument should cause them to question how ParseCalendar works without location information?

[arran4]

These solutions are not mutually incompatible although redundant and they all take time. I definitely think a function definition is part of what ever solution we adopt. So I'm happy with both just I have my preferences but this is about the users. :P

[brackendawson]

Spotted that #67 is related to this.

[arran4]

It's been over a year I wonder if @dnwe remember it even. I intend to adopt a couple PRs to get them through.

Would you say #67 resolves it?

[arran4]

Attempted to resolve the merge conflicts in: https://github.com/arran4/golang-ical/pull/101 not sure how that resolves this.

[arran4]

Looks like not. I will get to this eventually I have the other PRs I want to get across the line first.

[arran4]

I'll probably do something similar to what i did with the ops for this in: https://github.com/arran4/golang-ical/pull/106

Going to wait for that to go though

[arran4]

Actually no overlap! Please review: https://github.com/arran4/golang-ical/pull/107 Modify or what ever gets the best results.