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

Yearly rrule compliance by the iterator #656

Merged
merged 2 commits into from
May 17, 2024

Conversation

kroky
Copy link
Contributor

@kroky kroky commented May 14, 2024

Make the RRuleIterator always follow the yearly RRULE even when start date does not follow the rule. Should solve #626

Copy link

codecov bot commented May 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.78%. Comparing base (8bf65e2) to head (72a1fe9).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##             master     #656   +/-   ##
=========================================
  Coverage     98.78%   98.78%           
- Complexity     1868     1869    +1     
=========================================
  Files            71       71           
  Lines          5253     5254    +1     
=========================================
+ Hits           5189     5190    +1     
  Misses           64       64           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@phil-davis phil-davis self-requested a review May 15, 2024 07:48
Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

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

Just making a comment for now.
The code looks good, I will think a bit about if there are other RRULE scenarios that might behave badly...

'FREQ=YEARLY;BYMONTH=6;BYDAY=-1FR;UNTIL=20250901T000000Z',
'2023-09-01 12:00:00',
[
'2023-09-01 12:00:00',
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: the existing behavior is that this RRULE creates this event on 2023-09-01 and a recurrence on 2023-09-29, as well as the expected recurrences in June of 2024 and 2025.

IMO it is a separate issue about whether the first occurrence on 2023-09-01 should happen - the issue and this PR are not about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, having the first day not following the RRule pattern is a violation of the RFC, so maybe a validation error is needed somewhere...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, having the first day not following the RRule pattern is a violation of the RFC, so maybe a validation error is needed somewhere...

We are already being flexible and allowing the first/start day to not have to be a match for the RRULE pattern. And the code schedules that start day. So I suppose we had better keep that behavior, otherwise there would be a BC break.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the flexibility here, so +1

@phil-davis phil-davis self-requested a review May 17, 2024 06:47
@phil-davis
Copy link
Contributor

I added a couple more test scenarios just to cover when the specified start date is in a month before/the same as/after the month where the recurring event should happen. It passes.

Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

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

LGTM
I tried some similar-style combinations of MONTHLY rules with various "by day" specs and starting on a day that is not one of the days selected by the rule. All things that I tried resulted in reasonable recurrences being scheduled, so there does not seem to be any problem at the MONTHLY/BYDAY level.

@phil-davis phil-davis merged commit 254f85d into sabre-io:master May 17, 2024
8 checks passed
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.

None yet

2 participants