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.

Sign up for GitHub

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

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a4a86c6
Fix and comments.
arran4 Oct 15, 2024
e7f9728
Would have gotten away with it too if it weren't for those meddling t…
arran4 Oct 15, 2024
d3a83a4
Merge branch 'master' into issue99
arran4 Oct 16, 2024
35b9379
Merge branch 'master' into issue99
arran4 Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion calendar_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ func TestTimeParsing(t *testing.T) {
},
}

assertTime := func(evtUid string, exp time.Time, timeFunc func() (given time.Time, err error)) {
assertTime := func(evtUid string, exp time.Time, timeFunc func(ops ...any) (given time.Time, err error)) {
given, err := timeFunc()
if err == nil {
if !exp.Equal(given) {
Expand Down
83 changes: 61 additions & 22 deletions components.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"errors"
"fmt"
"io"
"reflect"
"regexp"
"strconv"
"strings"
Expand Down Expand Up @@ -263,11 +264,28 @@ func (cb *ComponentBase) SetDuration(d time.Duration) error {
return errors.New("start or end not yet defined")
}

func (cb *ComponentBase) GetEndAt() (time.Time, error) {
return cb.getTimeProp(ComponentPropertyDtEnd, false)
}

func (cb *ComponentBase) getTimeProp(componentProperty ComponentProperty, expectAllDay bool) (time.Time, error) {
// GetEndAt gets the date and time that the component finishes at, must not be an all day event.
// Warning: by default uses "Local" timezone -- To overwrite it, supply the desired timezone as one of the optional arguments. Ie, GetEndAt(time.UTC)
// If you have a property that specifies a timezone then that is used instead.
func (cb *ComponentBase) GetEndAt(ops ...any) (time.Time, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@brackendawson brackendawson Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Owner Author

@arran4 arran4 Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@brackendawson brackendawson Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return cb.getTimeProp(ComponentPropertyDtEnd, false, ops...)
}

// getTimeProp extracts a date and (optionally if not expectAllDay) time from the specified property.
// Warning: by default uses "Local" timezone -- To overwrite it, supply the desired timezone as one of the optional arguments. Ie, getTimeProp(..., time.UTC)
// If you have a property that specifies a timezone then that is used instead.
func (cb *ComponentBase) getTimeProp(componentProperty ComponentProperty, expectAllDay bool, ops ...any) (time.Time, error) {
fallBackTimezone := time.Local
for opi, op := range ops {
switch op := op.(type) {
case time.Location:
fallBackTimezone = &op
case *time.Location:
fallBackTimezone = op
default:
return time.Time{}, fmt.Errorf("%w: option %d: %s", ErrorUnsupportedOptionalArgument, opi, reflect.TypeOf(op))
}
}
timeProp := cb.GetProperty(componentProperty)
if timeProp == nil {
return time.Time{}, fmt.Errorf("%w: %s", ErrorPropertyNotFound, componentProperty)
Expand Down Expand Up @@ -303,7 +321,7 @@ func (cb *ComponentBase) getTimeProp(componentProperty ComponentProperty, expect
return time.ParseInLocation(icalDateFormatUtc, dateStr+"Z", time.UTC)
} else {
if propLoc == nil {
return time.ParseInLocation(icalDateFormatLocal, dateStr, time.Local)
return time.ParseInLocation(icalDateFormatLocal, dateStr, fallBackTimezone)
} else {
return time.ParseInLocation(icalDateFormatLocal, dateStr, propLoc)
}
Expand All @@ -318,15 +336,15 @@ func (cb *ComponentBase) getTimeProp(componentProperty ComponentProperty, expect
return time.ParseInLocation(icalTimestampFormatUtc, timeVal, time.UTC)
case grp1len > 0 && grp3len > 0 && tOrZGrp == "T" && zGrp == "":
if propLoc == nil {
return time.ParseInLocation(icalTimestampFormatLocal, timeVal, time.Local)
return time.ParseInLocation(icalTimestampFormatLocal, timeVal, fallBackTimezone)
} else {
return time.ParseInLocation(icalTimestampFormatLocal, timeVal, propLoc)
}
case grp1len > 0 && grp3len == 0 && tOrZGrp == "Z" && zGrp == "":
return time.ParseInLocation(icalDateFormatUtc, dateStr+"Z", time.UTC)
case grp1len > 0 && grp3len == 0 && tOrZGrp == "" && zGrp == "":
if propLoc == nil {
return time.ParseInLocation(icalDateFormatLocal, dateStr, time.Local)
return time.ParseInLocation(icalDateFormatLocal, dateStr, fallBackTimezone)
} else {
return time.ParseInLocation(icalDateFormatLocal, dateStr, propLoc)
}
Expand All @@ -335,20 +353,32 @@ func (cb *ComponentBase) getTimeProp(componentProperty ComponentProperty, expect
return time.Time{}, fmt.Errorf("time value matched but not supported, got '%s'", timeVal)
}

func (cb *ComponentBase) GetStartAt() (time.Time, error) {
return cb.getTimeProp(ComponentPropertyDtStart, false)
// GetStartAt Gets the time an event starts at, must not be an all day event
// Warning: by default uses "Local" timezone -- To overwrite it, supply the desired timezone as one of the optional arguments. Ie, GetStartAt(time.UTC)
// If you have a property that specifies a timezone then that is used instead.
func (cb *ComponentBase) GetStartAt(ops ...any) (time.Time, error) {
return cb.getTimeProp(ComponentPropertyDtStart, false, ops...)
}

func (cb *ComponentBase) GetAllDayStartAt() (time.Time, error) {
return cb.getTimeProp(ComponentPropertyDtStart, true)
// GetAllDayStartAt gets the start date (in time.Time) of an all day event. Must not have a "time" associated with it.
// Warning: by default uses "Local" timezone -- To overwrite it, supply the desired timezone as one of the optional arguments. Ie, GetAllDayStartAt(time.UTC)
// If you have a property that specifies a timezone then that is used instead.
func (cb *ComponentBase) GetAllDayStartAt(ops ...any) (time.Time, error) {
return cb.getTimeProp(ComponentPropertyDtStart, true, ops...)
}

func (cb *ComponentBase) GetLastModifiedAt() (time.Time, error) {
return cb.getTimeProp(ComponentPropertyLastModified, false)
// GetLastModifiedAt parses and returns the last modified date and time in time.Time format.
// Warning: by default uses "Local" timezone -- To overwrite it, supply the desired timezone as one of the optional arguments. Ie, GetLastModifiedAt(time.UTC)
// If you have a property that specifies a timezone then that is used instead.
func (cb *ComponentBase) GetLastModifiedAt(ops ...any) (time.Time, error) {
return cb.getTimeProp(ComponentPropertyLastModified, false, ops...)
}

func (cb *ComponentBase) GetDtStampTime() (time.Time, error) {
return cb.getTimeProp(ComponentPropertyDtstamp, false)
// GetDtStampTime gets the Dtstamp time date in time.Time format for a property
// Warning: by default uses "Local" timezone -- To overwrite it, supply the desired timezone as one of the optional arguments. Ie, GetDtStampTime(time.UTC)
// If you have a property that specifies a timezone then that is used instead.
func (cb *ComponentBase) GetDtStampTime(ops ...any) (time.Time, error) {
return cb.getTimeProp(ComponentPropertyDtstamp, false, ops...)
}

func (cb *ComponentBase) SetSummary(s string, params ...PropertyParameter) {
Expand Down Expand Up @@ -573,8 +603,11 @@ func (event *VEvent) Alarms() []*VAlarm {
return event.alarms()
}

func (event *VEvent) GetAllDayEndAt() (time.Time, error) {
return event.getTimeProp(ComponentPropertyDtEnd, true)
// GetAllDayEndAt gets the end date (in time.Time) of an all day event. Must not have a "time" associated with it.
// Warning: by default uses "Local" timezone -- To overwrite it, supply the desired timezone as one of the optional arguments. Ie, GetAllDayEndAt(time.UTC)
// If you have a property that specifies a timezone then that is used instead.
func (event *VEvent) GetAllDayEndAt(ops ...any) (time.Time, error) {
return event.getTimeProp(ComponentPropertyDtEnd, true, ops...)
}

type TimeTransparency string
Expand Down Expand Up @@ -696,12 +729,18 @@ func (c *VTodo) Alarms() []*VAlarm {
return c.alarms()
}

func (c *VTodo) GetDueAt() (time.Time, error) {
return c.getTimeProp(ComponentPropertyDue, false)
// GetDueAt parses and returns the date and time of a due date for a todo
// Warning: by default uses "Local" timezone -- To overwrite it, supply the desired timezone as one of the optional arguments. Ie, GetDueAt(time.UTC)
// If you have a property that specifies a timezone then that is used instead.
func (c *VTodo) GetDueAt(ops ...any) (time.Time, error) {
return c.getTimeProp(ComponentPropertyDue, false, ops...)
}

func (c *VTodo) GetAllDayDueAt() (time.Time, error) {
return c.getTimeProp(ComponentPropertyDue, true)
// GetAllDayDueAt gets the due date (in time.Time) of an vtodo. Must not have a "time" associated with it.
// Warning: by default uses "Local" timezone -- To overwrite it, supply the desired timezone as one of the optional arguments. Ie, GetAllDayDueAt(time.UTC)
// If you have a property that specifies a timezone then that is used instead.
func (c *VTodo) GetAllDayDueAt(ops ...any) (time.Time, error) {
return c.getTimeProp(ComponentPropertyDue, true, ops...)
}

type VJournal struct {
Expand Down
3 changes: 2 additions & 1 deletion errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,6 @@ import "errors"
var (
// ErrorPropertyNotFound is the error returned if the requested valid
// property is not set.
ErrorPropertyNotFound = errors.New("property not found")
ErrorPropertyNotFound = errors.New("property not found")
ErrorUnsupportedOptionalArgument = errors.New("unsupported time property type")
)
Loading