-
Notifications
You must be signed in to change notification settings - Fork 2
Implement RRule merging logic for ScheduleSet with iterative reduction #240
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
base: rrule
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot wasn't able to review any files in this pull request.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: jkrrv <1238391+jkrrv@users.noreply.github.com>
Co-authored-by: jkrrv <1238391+jkrrv@users.noreply.github.com>
Co-authored-by: jkrrv <1238391+jkrrv@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
jkrrv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few things here to adjust.
| $newRules = array_merge($newRules, $freqRules); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure that this is able to apply iteratively. That is, if a Yearly is able to be reduced to a Monthly, and the Monthly is able to be reduced to a weekly, then it should be reduced as far as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented iterative reduction in mergeIfPossible(). The method now loops and reapplies merge logic until no further reductions are possible, allowing Yearly -> Monthly -> Weekly cascading reductions. See commit 81d4791.
| * @param string|\DateTime $date Date string in RFC format or DateTime object | ||
| * @return string Week identifier (e.g., "2025-52") | ||
| */ | ||
| private function getWeekOfDate($date): string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation misses the point. For combining two series (weekly, monthly or yearly) it doesn't matter if they end the same week, but rather if there is an acceptable shared end date. That is, if series A's last occurrence is before series B's last occurrence, they can share an end date as long as the pattern of A wouldn't have another occurrence after the end of A and before the end of B.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the end date compatibility logic. Now properly checks if series A's pattern would have occurrences between A's end and B's end using the hasOccurrencesBetween() method, rather than just checking if they end in the same week. See commit 81d4791.
Co-authored-by: jkrrv <1238391+jkrrv@users.noreply.github.com>
Co-authored-by: jkrrv <1238391+jkrrv@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot Please expand unit tests to add test cases for the situations I described in the previous preview, and also for 100% coverage of the Schedule.php and ScheduleSet.php |
Co-authored-by: jkrrv <1238391+jkrrv@users.noreply.github.com>
Expanded unit tests with comprehensive coverage: Schedule.php tests (18 new tests):
ScheduleSet.php tests (21 new tests):
Results: 126 total tests (was 85), 274 assertions (was 208), all passing. See commit 576faae. |
Adds
mergeIfPossible()method to combine compatible recurrence rules, reducing redundancy in schedule representations. The method applies iteratively to reduce rules as far as possible (e.g., Yearly → Monthly → Weekly).Changes
Schedule class - Added getter methods exposing RRule properties:
getFreq(),getCount(),getByDay(),getByMonth(),getByHour(),getByMinute(),getBySecond(),getUntil(),getInterval()getUntil()normalizes DateTime objects to RFC 5545 format stringsScheduleSet class - Implemented intelligent rule merging with iterative reduction:
ScheduleSet constructor - Extended to accept arrays of Schedule/RRule objects
Comprehensive unit tests - Added extensive test coverage:
Example
Iterative Reduction Example
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.