Fix #729: incorrect expansion behaviour after BYWEEKNO
#731
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ ❌ Your project status has failed because the head coverage (66%) is below the target coverage (80%). You can increase the head coverage or adjust the target coverage. @@ Coverage Diff @@
## main #731 +/- ##
===================================
Coverage 66% 66%
===================================
Files 105 105
Lines 4646 4649 +3
Branches 1154 1151 -3
===================================
+ Hits 3067 3073 +6
+ Misses 1143 1142 -1
+ Partials 436 434 -2
|
minichma
force-pushed
the
work/minichma/bugfix/byweekno_expansion
branch
from
3ab7885 to
8f0643d
Compare
|
@axunonb Tried to slightly reduce cyclomatic complexity of |
| } | ||
| } | ||
|
|
||
| private sealed class ExpandContext |
There was a problem hiding this comment.
use a struct or struct record instead, or even a bool instead?
There was a problem hiding this comment.
Yes, could be, and pass it by ref. Thought about it but went for simplicity. The overhead is rather low, but agree, struct would be nicer nevertheless. Will check.
| RRULE:FREQ=YEARLY;BYYEARDAY=2;BYWEEKNO=53;UNTIL=20380102 | ||
| DTSTART:20270102 | ||
| INSTANCES:20270102,20330102,20380102 | ||
|
|
axunonb
left a comment
axunonb
left a comment
There was a problem hiding this comment.
Very nice! See minor remarks
I.e. fix `BYMONTH`, `BYMONTHDAY`, `BYYEARDAY` in relation to `BYWEEKNO`
… little happier.
minichma
force-pushed
the
work/minichma/bugfix/byweekno_expansion
branch
from
8f0643d to
050e753
Compare
|
Thanks for the review, improved as suggested. Nothing to see from you being not so responsive. ;-) Wish you a great journey! |
|
axunonb
left a comment
axunonb
left a comment
There was a problem hiding this comment.
Thanks for the wishes and the changes. Great!
The expansion of
BYMONTH,BYMONTHDAY,BYYEARDAYwas incorrect when used together withBYWEEKNO, which is fixed in this PR. Corresponding tests are added.Example test failing prior to the fix:
Closes #729