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.

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

fix: daylight savings #66

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

fix: daylight savings #66

wants to merge 3 commits into from

Conversation

Xyedo
Copy link

@Xyedo Xyedo commented May 6, 2024

fixing this issues: #63
and appending the testcase on #64:

By letting golang time package calculate the daylight savings by changing it to time.Duration and add it to the existing Date

@leaanthony-sc
Copy link

leaanthony-sc commented May 20, 2024

I've discovered an edge case where this does not work: If you have a DAILY frequency and it crosses a timeline, then it will fail.

Test added to rrule_test.go:

func TestDailyDST(t *testing.T) {
	sydney, _ := time.LoadLocation("Australia/Sydney")
	r, _ := NewRRule(ROption{
		Freq:    DAILY,
		Count:   3,
		Dtstart: time.Date(2023, 4, 1, 9, 0, 0, 0, sydney),
	})
	want := []time.Time{
		time.Date(2023, 4, 1, 9, 0, 0, 0, sydney),
		time.Date(2023, 4, 2, 9, 0, 0, 0, sydney),
		time.Date(2023, 4, 3, 9, 0, 0, 0, sydney),
	}
	value := r.All()
	if !timesEqual(value, want) {
		t.Errorf("get %v, want %v", value, want)
	}
}

Running it gives us the following error:

=== RUN   TestDailyDST
    rrule_test.go:1751: get [2023-04-01 09:00:00 +1100 AEDT 2023-04-02 08:00:00 +1000 AEST 2023-04-03 09:00:00 +1000 AEST], want [2023-04-01 09:00:00 +1100 AEDT 2023-04-02 09:00:00 +1000 AEST 2023-04-03 09:00:00 +1000 AEST]
--- FAIL: TestDailyDST (0.00s)

I believe for DAILY we need to recreate the date but only add the number of days.

Xyedo and others added 2 commits May 20, 2024 21:28
This reverts commit c969eda.

chore: revert back to original solution and add fixes
@Xyedo
Copy link
Author

Xyedo commented May 20, 2024

I think for FREQ hourly, minutely, and secondly we need to add with time duration, so the duration is fixed, but with FREQ Daily, Weekly, and Monthly, we follow the time provided by the DTSTART and then add the Date.

@leaanthony-sc
Copy link

This change appears to work. Awesome job @Xyedo! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants