fix(recur): apply BYHOUR, BYMINUTE, BYSECOND, BYYEARDAY and BYWEEKNO

[proscar87]

Fixes #678.

as_rrule() passed nine keyword arguments to dateutil.rrule.rrule(). That constructor accepts byhour, byminute, bysecond, byyearday and byweekno too, so the engine could already compute them — they were simply never handed over, while being parsed, stored and re-serialised. An event kept the hour from DTSTART and the file round-tripped unchanged, which is why this looked like it worked.

Measured before and after, DTSTART:20250715T140000Z:

RRULE	before	after
FREQ=DAILY;COUNT=3;BYHOUR=9,17	15th 14:00, 16th 14:00, 17th 14:00	15th 17:00, 16th 09:00, 16th 17:00
FREQ=YEARLY;COUNT=2;BYYEARDAY=200	2025-07-15	2025-07-19, 2026-07-19

Three places needed the new keys, and missing any one of them fails differently: the model, the rule parser that splits comma-separated lists (without it, BYHOUR=9,17 arrives as the string '9,17' and validation fails), and _as_rrule_str.

Tests

tests/types/test_recur_byparts.py, 13 cases. Each part is asserted against dateutil.rrule given the same arguments rather than against a hand-written list — the expansion engine is the same one this library uses, so a hand-written expectation would only restate whatever the implementation produced.

Also pinned: the parts still round-trip to ics, a rule using none of them is byte-identical to before, and negative BYYEARDAY counts from the end of the year.

Two things you should look at rather than take on trust

A test asserted the bug. test_recur_extra_fields_preservation checked that BYYEARDAY and BYWEEKNO arrive as unparsed extras — a description of this defect, written when they weren't modelled. Left alone it would have failed; changed carelessly it would have stopped testing anything. It now asserts they parse, and that a genuinely unknown key (X-EXTRA) is still preserved as an extra, which is what the test was really for.

Snapshots are regenerated, because Recur's repr gained five fields. I checked that no occurrence actually changed: every dtstart in the updated snapshots is identical to the one it replaced — the diff is only the five empty lists appearing in the repr.

CI note

pre-commit run ty fails on ical/recur_adapter.py:84 with an unused-ignore warning. That is present on an untouched main — verified against a clean worktree of upstream/main — and is unrelated to this change, so I have not touched it. Every other hook passes. Suite: 1469 passed, 35 skipped.

🤖 Generated with Claude Code

[allenporter]

This belongs in test_recur.py. We add tests in the module they are testing, not arbitrarily named files that AI likes to generate.

[proscar87]

Reviewing my own commit turned up a defect I had introduced, plus a valid rfc5545 value I would have broken. Both fixed in the second commit — flagging rather than force-pushing over it, because one of them is a behaviour change you may want decided differently.

Passing these parts through meant out-of-range values now reach dateutil, which raises during iteration:

BYHOUR=99 -> ValueError: hour must be in 0..23, not 99

Before this PR, BYHOUR=99 was silently ignored and nothing failed. With the first commit alone, it parsed cleanly and then took down the expansion — the same shape as the UNTIL defect in #676, which I'd just finished arguing against. I'd have shipped the bug I was fixing elsewhere.

Now validated on the field, which is what by_month_day and by_setpos already do for the same reason, and puts the error where the bad value is.

BYSECOND=60 is a valid rfc5545 value that dateutil rejects — the spec permits 60 for a leap second, dateutil insists on 0..59. My own field docstring said so while the code would have raised on it. It now folds onto 59 rather than failing a calendar over something the spec allows.

The behaviour change worth your opinion

A malformed BYHOUR=99 went from silently ignored to CalendarParseError. That is stricter than before this PR.

I chose it for consistency — BYMONTHDAY=99 already raises today, so modelling these five parts puts them in a family with an established rule. But the library is deliberately lenient elsewhere, and you may prefer dropping out-of-range values with a debug log instead, which would keep such files loading. Say the word and I'll change it; it is a few lines either way.

18 tests now. Five fail without the validators, and the rest of the PR's cases are unaffected. Suite 1474 passed, 35 skipped; all hooks green except the pre-existing ty warning on recur_adapter.py noted above.

🤖 Generated with Claude Code

[allenporter]

Thanks, proposed solution looks fine by me.