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

RRULE evaluation: Ignore BYtime if DTSTART is date-only #619

Merged

Conversation

minichma
Copy link
Collaborator

According to RFC 5545, when evaluating an RRULE based on a DTSTRT of type DATE (i.e. date-only), any BY[SECOND|MINUTE|HOUR] MUST be ignored. This is implemented by this PR.

The BYSECOND, BYMINUTE and BYHOUR rule parts MUST NOT be specified
when the associated "DTSTART" property has a DATE value type.
These rule parts MUST be ignored in RECUR value that violate the
above requirement(e.g., generated by applications that pre - date
this revision of iCalendar).

This fixes the case DTSTART:20241018, RRULE:FREQ=DAILY;BYMINUTE=1,2,3,4;INTERVAL=2;COUNT=3 of #618.

Copy link

@minichma minichma marked this pull request as ready for review October 20, 2024 19:15
@minichma minichma requested a review from axunonb October 22, 2024 13:52
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.

Nicely commented,, important fix, tnx

@axunonb
Copy link
Collaborator

axunonb commented Oct 23, 2024

@minichma Feel free to merge the 2 approved PRs after considering whether the remarks are useful or not.

The only thing I'm pretty much convinced by now is: we need more coverage of use cases from the wild, which results in more effort than code coverage. Thanks for the structured approach you already started.
BTW: Would CI based code coverage integration be useful at this time, and if so: What should be the minimum coverage and code quality benchmark for PRs?

@minichma
Copy link
Collaborator Author

Regarding test cases for this fix I was running this based on #617. I think we should consider merging #617 and introducing our own test files in the same format. This would allow us adding new tests very easily and also to update test cases from the libical project. Then we'd also have specific coverage for this PR.

@minichma
Copy link
Collaborator Author

Would CI based code coverage integration be useful at this time, and if so: What should be the minimum coverage and code quality benchmark for PRs?

I'm pretty sure it would. Most important would be that the overall coverage would increase over time. Imposing high quality figures would only be helpful if we also have a reasonable path to get there.

@minichma minichma merged commit 36f2666 into ical-org:main Oct 24, 2024
2 checks passed
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