Fixes for RRULE:FREQ=YEARLY combined with BYMONTH
#884
Conversation
`RecurrencePatternEvaluator`: 1. Extended `GetIntervalLowerLimit` calculation: * YEARLY + BYMONTH: compute a conservative earliest possible candidate inside the interval * YEARLY + BYWEEKNO: shift interval start to the week’s first day * default return intervalRefTime 2. Intoduced an `anchorMonth` as an explicit month anchor used when FREQ=YEARLY and BYMONTH is not specified so that BYMONTHDAY expansion/limiting is performed relative to the original DTSTART month rather than the current intervalRefTime month. `anchorMonth` only affects cases where BYMONTH is absent. `Evaluator`: Added a distinction for case `FrequencyType.Yearly`: Is BYWEEKNO present or not. `RecurrenceTests`: Extended `YearlyInterval1()` to test more occurrences and taking latest code changes into account. Fixes #791 (comment)
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #884 +/- ##
=======================================
+ Coverage 68.2% 68.3% +0.1%
=======================================
Files 115 115
Lines 4357 4380 +23
Branches 1007 1013 +6
=======================================
+ Hits 2970 2991 +21
Misses 1028 1028
- Partials 359 361 +2
🚀 New features to boost your workflow:
|
…thDayVariantsLimited`
There was a problem hiding this comment.
Pull request overview
This PR refactors the recurrence pattern evaluation logic to correctly handle YEARLY frequency rules, particularly when BYMONTH is not explicitly specified. The key insight is that when BYMONTH is absent in a YEARLY rule, BYMONTHDAY expansion/limiting should be performed relative to the original DTSTART month rather than the current interval reference month.
Key Changes:
- Extended
GetIntervalLowerLimitto compute conservative earliest possible candidates for YEARLY+BYMONTH rules and handle BYWEEKNO week boundary cases - Introduced
anchorMonthparameter to ensure BYMONTHDAY limiting uses the correct month reference when BYMONTH is not specified - Modified
IncrementDatefor YEARLY frequency to preserve month/day when BYWEEKNO is absent, only resetting to January 1st when BYWEEKNO is present
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| Ical.Net/Evaluation/RecurrencePatternEvaluator.cs | Refactored GetIntervalLowerLimit with comprehensive logic for YEARLY rules; added anchorMonth parameter to GetCandidates and GetMonthDayVariants* methods; improved code style (removed unnecessary else, modernized tuple syntax); fixed spelling error |
| Ical.Net/Evaluation/Evaluator.cs | Distinguished YEARLY increment behavior based on BYWEEKNO presence - preserves month/day when absent, resets to Jan 1 when present |
| Ical.Net.Tests/RecurrenceTests.cs | Updated YearlyInterval1 test expectations to reflect correct behavior (occurrences in April matching DTSTART month instead of January); improved assertion style; removed trailing comma |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
axunonb
changed the title
RecurrencePatternEvaluator and EvaluatorRRULE:FREQ=YEARLY combined with BYMONTH
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
axunonb
force-pushed
the
wip/axunonb/pr/recurrence-pattern-evaluator
branch
from
125e071 to
84eb777
Compare
|
minichma
left a comment
minichma
left a comment
There was a problem hiding this comment.
I'm obviously a little late as you just merged already. Anyhow, here are my findings.
| dt = old.AddDays(-old.DayOfYear + 1).AddYears(interval); | ||
| // When a rule uses BYWEEKNO, recurrence enumeration | ||
| // is based on week numbers relative to the year. | ||
| // So we preserve January 1st when adding years; otherwise, preserve month/day. |
There was a problem hiding this comment.
Maybe more precisely
So we preserve the weekday when using BYWEEKNO and preserve month/day otherwise?
There was a problem hiding this comment.
Yes, this is more precise.
| if (pattern.ByMonthDay.Count > 0) | ||
| { | ||
| var md = pattern.ByMonthDay.Min(); | ||
| day = md > 0 ? Math.Min(md, daysInMonth) : Math.Max(1, daysInMonth + md + 1); |
There was a problem hiding this comment.
Probably not correct if positive/negative values are mixed, e.g. BYMONTHDAY=2,-2.
| return intervalLowerLimit; | ||
| switch (pattern) | ||
| { | ||
| case { Frequency: FrequencyType.Yearly, ByMonth.Count: > 0, ByWeekNo.Count: 0 }: |
There was a problem hiding this comment.
I feel this whole method is not perfectly intuitive, maybe we should improve the naming or the description. I.e. if I remember correctly, it is there for a minor performance improvement, i.e. to avoid iterating an extra year at the end of the sequence where this can be avoided, maybe also to avoid running into a EvaluationLimitExceededException in certain cases. However, for some reason this is only implemented for the case of BYWEEKNO. I assume this has historic reasons and might be related to the fact that in case of BYWEEKNO we might need to step back to the previous interval (at least when implemented the way it is today). For all other cases we don't do this extra calculation, which brings up the question whether we should do it for this new special case. I'd rather say no, i.e. I'd rather recommend implementing it for all cases (which I'd rather try to avoid) or only for the minimum set of cases necessary. But I could also easily miss something.
There was a problem hiding this comment.
For all other cases we don't do this extra calculation, which brings up the question whether we should do it for this new special case. I'd rather say no, i.e. I'd rather recommend implementing it for all cases (which I'd rather try to avoid)
Yes, agree, that's not clean. Covering special cases here is a risk. Not sure, which further special cases we can expect besides those just implemented. For the moment, they do their job.
| // (the expected behavior for yearly rules without an explicit BYMONTH), instead of the | ||
| // current interval reference month. | ||
| var anchorMonth = pattern is { Frequency: FrequencyType.Yearly, ByMonth.Count: 0 } | ||
| ? originalDate.Month |
There was a problem hiding this comment.
When changing this to return intervalRefTime.Month in both cases, all tests still run through. Would be great to have a test case where this distinction is essential.
There was a problem hiding this comment.
The anchorMonth was one of the ideas introduced in a very first draft. It was only partly helpful. When investigating further, anchorMonth became fully redundant - which I oversaw. Excellent finding, Please keep up these reviews.
| else | ||
| { | ||
| // When BYMONTH is not specified, only consider dates in the anchor month. | ||
| if (date.Month != anchorMonth) |
There was a problem hiding this comment.
Would be good to add some more context on why this was added. Not sure it is fully correct. Consider the following test case, which fails now (could be added to RecurrenceTestCases.txt if considered correct).
RRULE:FREQ=YEARLY;BYYEARDAY=-1,1;BYMONTHDAY=-1,1;COUNT=3
DTSTART:20250101
INSTANCES:20250101,20251231,20260101There was a problem hiding this comment.
Yes, I consider the test correct. It worked before this commit. Sadly we didn't have the test before - so thanks a lot.
I wonder whether using an anchor month has even more side effects than this one.
Anyway, removing if (date.Month != anchorMonth) continue; makes sense, and all tests including the new one above pass.
|
It's never too late to improve, thanks a lot |
As pointed out in the review for PR #884 Included unit test to verify
* Fix regression introduced in v5.1.3 Implemented review comments for PR #884 by @minichma * Correction in `RecurrencePatternEvaluator.GetIntervalLowerLimit` As pointed out in the review for PR #884 Included unit test to verify * Implement review comments Restore former behavior in `Evaluator.IncrementDate`: `case FrequencyType.Yearly: dt = old.AddDays(-old.DayOfYear + 1).AddYears(interval);` `RecurrencePatternEvaluator.GetIntervalLowerLimit` For yearly FREQUENCY but no BYMONTH or BYWEEKNO use the original DTSTART's month for interval boundary, preventing occurrences earlier than the intended range. Move test cases of this PR to `RecurrenceTestCases.txt` Added tests mentioned as failing in the review
Fixes for
RRULE:FREQ=YEARLYcombined withBYMONTHRecurrencePatternEvaluator:GetIntervalLowerLimitcalculation:anchorMonthas an explicit month anchor used when FREQ=YEARLY and BYMONTH is not specified so that BYMONTHDAY expansion/limiting is performed relative to the original DTSTART month rather than the current intervalRefTime month.anchorMonthonly affects cases where BYMONTH is absent.Evaluator:Added a distinction for case
FrequencyType.Yearly: Is BYWEEKNO present or not.RecurrenceTests:Extended
YearlyInterval1()to test more occurrences and taking latest code changes into account.See comments in #791 (comment) for reference
Fixes #885