Skip to content

Implement BYDAY with offset and limiting behavior#887

Merged
axunonb merged 5 commits intomainfrom
wip/axunonb/pr/byday-with-bymonthday-or-byyearday
Dec 23, 2025
Merged

Implement BYDAY with offset and limiting behavior#887
axunonb merged 5 commits intomainfrom
wip/axunonb/pr/byday-with-bymonthday-or-byyearday

Conversation

@axunonb
Copy link
Collaborator

@axunonb axunonb commented Dec 1, 2025

The following RRULE cases were not implemented (see the diff for newly added unit tests for details):

  1. YEARLY + BYMONTH + numeric BYDAY offsets

    • Pattern: FREQ=YEARLY;BYMONTH=6,9;BYDAY=2MO
    • Semantics: treat numeric BYDAY as “nth weekday inside each BYMONTH” (produce the 2nd Monday of each specified month).
  2. YEARLY + numeric BYDAY without BYMONTH

    • Pattern: FREQ=YEARLY;BYDAY=20MO
    • Semantics: interpret numeric BYDAY as “nth weekday of the YEAR” (the 20th Monday of the year).
  3. YEARLY + BYMONTH + negative numeric BYDAY

    • Pattern: FREQ=YEARLY;BYMONTH=6,9;BYDAY=-1SU
    • Semantics: negative offsets select from the end of the month (e.g. -1SU → last Sunday in each BYMONTH).
  4. Implemented the per-month offset expansion and preserved BYWEEKNO / BYMONTH compatibility and negative-offset

These behaviors are closely related and required special handling in RecurrencePatternEvaluator.GetAbsWeekDays and GetAbsWeekDaysYearlyPerMonthOffsets.

[Edit]
Relevant unit tests in RecurrenceTestCases.txt contain a disclaimer regarding RFC5545 interpretation in ical.net

Fixes #782
Closes #888

@codecov
Copy link

codecov bot commented Dec 1, 2025

Codecov Report

❌ Patch coverage is 94.82759% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
Ical.Net/Evaluation/RecurrencePatternEvaluator.cs 94.6% 1 Missing and 2 partials ⚠️

Impacted file tree graph

@@           Coverage Diff           @@
##            main    #887     +/-   ##
=======================================
+ Coverage   68.3%   68.5%   +0.2%     
=======================================
  Files        115     115             
  Lines       4376    4394     +18     
  Branches    1010    1011      +1     
=======================================
+ Hits        2988    3008     +20     
  Misses      1028    1028             
+ Partials     360     358      -2     
Files with missing lines Coverage Δ
Ical.Net/Evaluation/RecurrenceUtil.cs 100.0% <100.0%> (ø)
Ical.Net/Evaluation/RecurrencePatternEvaluator.cs 92.3% <94.6%> (+1.2%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@axunonb axunonb force-pushed the wip/axunonb/pr/byday-with-bymonthday-or-byyearday branch from 303983c to ce242db Compare December 1, 2025 22:08
@axunonb axunonb requested a review from minichma December 1, 2025 22:20
@axunonb axunonb marked this pull request as ready for review December 1, 2025 22:24
@maknapp
Copy link
Collaborator

maknapp commented Dec 9, 2025

FYI, I made several tests related to this for a new recurrence evaluator. I ran those tests against this fix and they all pass.

@axunonb axunonb force-pushed the wip/axunonb/pr/byday-with-bymonthday-or-byyearday branch from e218233 to 757f9fc Compare December 10, 2025 07:57
@axunonb axunonb requested a review from maknapp December 10, 2025 07:59
maknapp
maknapp previously approved these changes Dec 10, 2025
@ical-org ical-org deleted a comment from byteorbix Dec 10, 2025
@axunonb
Copy link
Collaborator Author

axunonb commented Dec 10, 2025

@maknapp Thanks for the review and the helpful hint

I made several tests related to this for a new recurrence evaluator. I ran those tests against this fix and they all pass.

So it looks like we're on the right way. As @minichma opened the issue, I'd like have his feedback as well, if he finds the time. He also contributes to libical.

@axunonb axunonb force-pushed the wip/axunonb/pr/byday-with-bymonthday-or-byyearday branch 2 times, most recently from f276890 to d2dc0dd Compare December 13, 2025 13:32
@axunonb
Copy link
Collaborator Author

axunonb commented Dec 13, 2025

@minichma Unit test for this PR are now part of RecurrenceTestCases.txt

The following `RRULE` cases were not implemented (see the diff for newly added unit tests for details):

1. YEARLY + BYMONTH + numeric BYDAY offsets
   - Pattern: `FREQ=YEARLY;BYMONTH=6,9;BYDAY=2MO`
   - Semantics: treat numeric BYDAY as “nth weekday inside each BYMONTH” (produce the 2nd Monday of each specified month).

2. YEARLY + numeric BYDAY without BYMONTH
   - Pattern: `FREQ=YEARLY;BYDAY=20MO`
   - Semantics: interpret numeric BYDAY as “nth weekday of the YEAR” (the 20th Monday of the year).

3. YEARLY + BYMONTH + negative numeric BYDAY
   - Pattern: `FREQ=YEARLY;BYMONTH=6,9;BYDAY=-1SU`
   - Semantics: negative offsets select from the end of the month (e.g. `-1SU` → last Sunday in each BYMONTH).

4. Implemented the per-month offset expansion and preserved BYWEEKNO / BYMONTH compatibility and negative-offset

These behaviors are closely related and required special handling in `RecurrencePatternEvaluator.GetAbsWeekDays` and `GetAbsWeekDaysYearlyPerMonthOffsets`.

Fixes #782
Updated recurrence yearly offset tests to use hardcoded CalDateTime values instead of CalCalc helper methods. Removed CalCalc.cs as its methods are no longer referenced.
Move tests are for RFC errata 1913 (https://www.rfc-editor.org/errata_search.php?rfc=1913&amp;eid=1913)
and contain the disclaimer "Other iCalendar libraries may interpret this differently"
@axunonb axunonb force-pushed the wip/axunonb/pr/byday-with-bymonthday-or-byyearday branch from c48b1c2 to 7d556e8 Compare December 17, 2025 13:02
Copy link
Collaborator

@minichma minichma left a comment

Choose a reason for hiding this comment

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

I started looking at the test cases and from my point of view they look good. Commented some minor thoughts, just nits. Didn't look at the actual code so far. Will try to find time at the weekend.


# Yearly BYMONTH numeric BYDAY (2MO) - 2nd Monday in each BYMONTH
RRULE:FREQ=YEARLY;BYMONTH=6,9;BYDAY=2MO;COUNT=4
DTSTART:20260601T090000
Copy link
Collaborator

Choose a reason for hiding this comment

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

DTSTART is not in line with the RRULE, which makes the case undefined according to the RFC (similar in other test cases). Unless this test should explicitly cover the undefined case, it would be good to have DTSTART aligned with RRULE, otherwise readers would have to think about whether the deviation is intentional.

The time part could be omitted, which would make the test somewhat more concise.

minichma
minichma previously approved these changes Dec 19, 2025
Copy link
Collaborator

@minichma minichma left a comment

Choose a reason for hiding this comment

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

Very nice! Sorry for the delay!

@sonarqubecloud
Copy link

@axunonb
Copy link
Collaborator Author

axunonb commented Dec 23, 2025

@minichma, @maknapp Thanks for your reviews, and happy holidays.

@axunonb axunonb merged commit e06dd22 into main Dec 23, 2025
12 checks passed
@axunonb axunonb deleted the wip/axunonb/pr/byday-with-bymonthday-or-byyearday branch December 23, 2025 21:57
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.

Use RecurrenceTestCases.txt as input for recurrence tests BYDAY with offset and limiting behavior not implemented

3 participants