Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix some recurrence evaluation issues #645

Merged
merged 3 commits into from
Nov 21, 2024
Merged

Conversation

minichma
Copy link
Collaborator

This PR fixes the following issues listed in #618:

DTSTART:20141006T090000, RRULE:FREQ=WEEKLY;INTERVAL=1;COUNT=2;BYDAY=MO;

The trailing semicolon caused an exception and is now ignored. Additionally handling of illegal rule parts having a number of = chars that is different from 1. In such a case an ArgumentException is raised, which seems to be in line with other exceptions raised in that class but could be discussed.

RRULE:FREQ=WEEKLY;UNTIL=20170127T000000Z;WKST=MO;BYDAY=SU,TU,TH;INTERVAL=2

The start of the week was not considered correctly.

Copy link

codecov bot commented Nov 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Impacted file tree graph

@@         Coverage Diff         @@
##           main   #645   +/-   ##
===================================
  Coverage    60%    60%           
===================================
  Files       100    100           
  Lines      4702   4707    +5     
  Branches   1102   1104    +2     
===================================
+ Hits       2830   2836    +6     
  Misses     1386   1386           
+ Partials    486    485    -1     
Files with missing lines Coverage Δ
Ical.Net/Evaluation/RecurrencePatternEvaluator.cs 73% <100%> (+<1%) ⬆️
...alization/DataTypes/RecurrencePatternSerializer.cs 70% <100%> (+<1%) ⬆️

🚨 Try these New Features:

@minichma minichma marked this pull request as ready for review November 20, 2024 23:49
Copy link
Collaborator

@axunonb axunonb left a comment

Choose a reason for hiding this comment

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

Great! Just a minor remark re Exception details

Copy link

sonarcloud bot commented Nov 21, 2024

@axunonb axunonb self-requested a review November 21, 2024 20:14
@minichma minichma merged commit c07dd44 into main Nov 21, 2024
6 of 7 checks passed
@minichma minichma deleted the bugfix/icalrecur_test_cases branch November 21, 2024 20:21
@minichma
Copy link
Collaborator Author

@axunonb How should we deal with merging into v5? 3-way merge? Cherry-pick? Separate PR? I think I'd vote for the 3-way merge.

@axunonb
Copy link
Collaborator

axunonb commented Nov 21, 2024

Yes, the 3-way merge would be beneficial. But would updating main besides version/v5 aim for further v4 releases? Otherwise we could freeze v4.

@minichma
Copy link
Collaborator Author

Oh, I was unsure, about your plans regarding v4. Given the creation of a v5 branch I thought main would continue to exist for v4 for now, but I agree that concentrating on v5 would be beneficial. But, if we don't plan for further v4 releases anyways, why not continue developing v5 on main? And in case we think another v4 is needed, we would create a v4 branch (or do that right now). That's how I know it from other project like libical too.

@axunonb
Copy link
Collaborator

axunonb commented Nov 22, 2024

Really appreciate your input! Of course, that makes perfect sense.

@axunonb
Copy link
Collaborator

axunonb commented Nov 22, 2024

Created version/v4 and then merged version/v5 into main

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.

2 participants