-
Notifications
You must be signed in to change notification settings - Fork 231
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
Test 'Evaluate1' broken - should fail but succeeds #602
Comments
Hey, very good! Thanks for reviewing.
This what I assume, too
Agree, it is prone to errors using "magic numbers" (0). The first idea, to use a nullable TimeSpan as an instance variable is not enough. |
Well, according to the doc-comment the |
Another problem with today's implementation of I feel that all this should be cleaned up in a v5, where we don't need to deal with backwards-compatibility. Otherwise this would become overly challenging. |
iCalaendar Validator considers the ics string from the test method as valid :)
|
Hm, but which occurrences would be expected? libical doesn't handle this case cleanly either. They return 5 recurrences, everyone date-only 20241016. |
I have asked them for their opinion: Hi,
|
What client apps do with it:
This might be an indication for no occurrences recognized or it's because of no duration. Co-Pilot says: The provided VEVENT in the VCALENDAR file specifies a recurrence rule (RRULE) with the following properties: The 5 occurrences will be:
|
Tried to find the relevant spec in the RFC but couldn't. I don't think this case is explicitly specified. A related topic is defined as follows:
So specifying We should either
I suggest to rewrite with a defined (maybe still forbidden) rule. I'd suggest something like
|
Reading the rule, and seeing what apps are doing, this sounds reasonable. What you're saying is: There is date, but no time, so it resolves to daily. Agree with your proposal, and write some comments regarding the dilemma and our decision. |
Just to be clear, I'm suggesting to change the test from something that's presumably forbidden and undefined (FREQ=MINUTELY) to something that's forbidden and defined (FREQ=DAILY;BYMINUTE=1,2,3). I did not comment on how ical.net should deal with the undefined case. (In fact I think the whole rrule should be rejected (ignored?). But that's a different story.) I also sent this as a question to the IETF Calsify mailing list.
|
Yes, understood. |
Thats what I received from Neil Jenkins on the calsify mailing list:
My updated suggestion would now be the following:
|
The test 'Evaluate1' is broken, as it tests for occurrences' properties but doesn't produce any. This is a improved version of the test. The relevant difference is the start date passed to
GetOccurrences
is fixed andAssert.AreEqual(5, occurrences.Count)
is added. The fixed version fails on current main branch.However, I'm not sure, whether the case covered in that test is allowed at all but I assume it isn't. I.e. the
MINUTELY
freq requires a time component, which doesn't exist in the given case, becauseevt.Start
is date-only. The comment says 'Start at midnight, UTC time', but without a time component, there is no real start date. Maybe it wasn't intended to have a date-only start, so then the problem would be caused by this, which is a problem closely related to #564.The text was updated successfully, but these errors were encountered: