Opened by mistake, please ignore - #86
Closed
aaazlkm wants to merge 1 commit into
Closed
Conversation
Restore WKST support that was previously removed (the golden tests were left commented out with a "re-add when we support WKST again" TODO): - iteration: use RecurrenceRule.actualWeekStart for the weekly date-set boundary (date_set.dart) and the weekly interval snap-back (frequency_interval.dart) instead of hardcoded Monday - decoder: accept any WKST weekday and actually pass the parsed value to the RecurrenceRule constructor (it was previously dropped) - model: relax the Monday-only constructor assert; include weekStart in ==/hashCode; add weekStart/clearWeekStart to copyWith - re-enable the commented RFC 5545 WKST tests, including the canonical "days generated makes a difference because of WKST" pair; fix a typo in the previously commented golden (RFC lists February 3,17 - not 18) - add dateutil-derived WKST goldens, codec round-trip, equality, and copyWith tests Limitation (documented on RecurrenceRule.weekStart): BYWEEKNO week numbering still uses ISO 8601 (Monday-based) weeks regardless of WKST, matching rrule.js behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR restores and extends WKST (week start) support for weekly recurrence rules across parsing, iteration, and the RecurrenceRule model, and re-enables/expands the previously commented-out golden tests.
Changes:
- Use
actualWeekStartfor weekly bucketing and weekly interval “snap-back” logic. - Decode and persist
WKSTfor any weekday; relax model validation and includeweekStartin equality/hashCode andcopyWith. - Re-enable and add WKST-focused goldens and round-trip/equality/copyWith tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/rrule_ical_test.dart | Re-enables RFC 5545 WKST goldens and updates expected outputs and test data structures. |
| test/recurrence_rule_test.dart | Adds WKST-specific unit tests (goldens, round-trips, equality/hashCode, copyWith). |
| lib/src/recurrence_rule.dart | Updates WKST validation/docs; adds weekStart to equality/hashCode and copyWith. |
| lib/src/iteration/frequency_interval.dart | Applies actualWeekStart when computing weekly interval advancement. |
| lib/src/iteration/date_set.dart | Applies actualWeekStart when building weekly date-set boundaries. |
| lib/src/codecs/string/decoder.dart | Accepts and passes through any valid WKST weekday instead of rejecting non-Monday values. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
237
to
255
| @override | ||
| int get hashCode { | ||
| return Object.hash( | ||
| frequency, | ||
| until, | ||
| count, | ||
| interval, | ||
| bySeconds, | ||
| byMinutes, | ||
| byHours, | ||
| byWeekDays, | ||
| byMonthDays, | ||
| byYearDays, | ||
| byWeeks, | ||
| byMonths, | ||
| bySetPositions, | ||
| weekStart, | ||
| ); | ||
| } |
aaazlkm
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request was opened by mistake while I was working on my fork.
It was not intended for the upstream repository and has been closed.
Please ignore this pull request.
Apologies for the noise and confusion.
Original auto-generated description
Summary
This restores
WKSTsupport for weekly frequencies — the golden tests for it were left commented out inrrule_ical_test.dartwith aTODO(JonasWanke): Re-add these tests when we support WKST again, so I hope this is welcome!Changes
actualWeekStartinstead of hardcodedDateTime.mondayin the two weekly-bucketing sites — the weekly date-set boundary (date_set.dart) and the weekly interval snap-back (frequency_interval.dart).WKSTweekday, and actually pass the parsed value to theRecurrenceRuleconstructor (it was previously parsed but dropped).isValidRruleDayOfWeek; includeweekStartin==/hashCode; addweekStart/clearWeekStarttocopyWith.WKST=MO→ Aug 5,10,19,24 vsWKST=SU→ Aug 5,17,19,31). One fix: the previously commented golden for "Every other week - forever" listed February 18, but RFC 5545's own expected output is "February 3,17" (Feb 17 1998 is the Tuesday) — corrected with a comment. Also added python-dateutil-derived goldens (testWkStIntervalMO/SUequivalents), aWKST=TUcase, string-codec round-trips for all seven weekdays, and equality/copyWithtests.Verification beyond the unit tests
The weekly expansion was verified differentially against Android CalendarProvider's Instances table (the platform's own RRULE engine): identical instance lists for
WKST=MO/SU/TUweekly rules atINTERVAL=1/2/3with variousBYDAYsets.Known limitation (documented on
weekStart)BYWEEKNOweek numbering still uses ISO 8601 (Monday-based) weeks regardless ofWKST— theWeekInfoalgorithm is unchanged. This matches rrule.js's level of WKST support.Full test suite passes (507 tests).