-
Notifications
You must be signed in to change notification settings - Fork 233
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
UNTIL property in RRULE is supposed to be inclusive #121
Comments
For context, I am calling RecurrencePatternEvaluator directly to evaluate recurrence pattern strings. |
OK there's a couple of things here:
If I am misunderstanding your issue, please post a complete VEVENT text block. |
OK, I've replicated your problem. But it's not a bug, it's a problem with how you have formulated your search range. Have a look. Here's the starting VEVENT:
Or, the programmatic equivalent, if you prefer: var rrule = new RecurrencePattern(FrequencyType.Weekly, interval: 1)
{
Until = DateTime.Parse("2016-08-31T07:00:00"),
ByDay = new List<IWeekDay> { new WeekDay(DayOfWeek.Wednesday)},
};
var start = DateTime.Parse("2016-08-01T07:00:00");
var end = start.AddHours(1);
var e = new Event()
{
DtStart = new CalDateTime(start, "UTC"),
DtEnd = new CalDateTime(end, "UTC"),
RecurrenceRules = new List<IRecurrencePattern> { rrule },
Summary = "This is an event",
Uid = "abab717c-1786-4efc-87dd-6859c2b48eb6",
};
var calendar = new Calendar();
calendar.Events.Add(e);
//Read the ical text, and make sure the programmatic composition and the raw text produce
//equivalent Calendars
var collection = Calendar.LoadFromStream(new StringReader(ical));
var firstEvent = collection.First().Events.First();
Assert.AreEqual(e, firstEvent); Here's the piece you care about, building on the above: // Midnight on July 1, 2016 is start of the search range
var startSearch = new CalDateTime(DateTime.Parse("2016-07-01T00:00:00"), "UTC");
// You are constraining the end of your search to Aug 31 @ 00:00, which is before Aug 31 @ 08:00
// If you extend the range to 2016-08-31T07:00:01 and on, the occurrence you expect will show up
var endSearch = new CalDateTime(DateTime.Parse("2016-08-31T07:00:01"), "UTC"); //Yes
var occurrences = firstEvent.GetOccurrences(startSearch, endSearch)
.Select(o => o.Period as Period)
.OrderBy(p => p.StartTime)
.ToList(); If you make A couple of thoughts:
That last piece can be re-evaluated, but I think it's OK as-is. This is one of the problems with time. If you have an event that starts at Aug 5 @ 9:00am, and you ask about events that occur between midnight on Aug 1 and Aug 5 @ 9:00am, should the search result include the thing that starts at 9am? It depends who you ask, but I lean towards "no". As a general rule of thumb, using dday.ical as a source of truth for anything is a really bad idea, because it's buggy throughout. It's a useful reference point, but not a source of truth. |
I added a unit test that asserts that there must be some overlap for an occurrence to show up in a recurrence set. There was no bug, but it doesn't hurt to assert implicit rules. Actually, I should probably add that detail to the |
Better intellisense docs: 8f691ac |
We use the library to express recurrence patterns for rules in our software's scheduling engine in a syntactically standard way that has supporting libraries on multiple platforms. We're only using the recurrence part, so we're not actually dealing with VEVENTs at all. Additionally, we also only deal with dates, so the 7AM UTC coming in is a bug on our side. Here's what we're calling (this is based off a code sample from Doug Day for a similar scenario): RecurrencePattern rp = new RecurrencePattern(recurrenceRule); It works if we strip the DTSTART and UNTIL out of the pattern - does this change how it's evaluated internally? The RRULE here is just "RRULE:FREQ=WEEKLY;INTERVAL=1;BYDAY=WE" with searchStart and searchEnd, and reference date set to searchStart. Since we're dealing with dates, when someone asks "I want dates from 8/1 to 8/31" the 8/31 is included. I agree with how you've defined the boundary between events that have time, and I'm fine with none of this resulting in code changes since our use case is nonstandard (we're handling this by adding 1ms to the searchEnd date to make the end inclusive). It sounds like DDay.iCal returning 8/31 when DTSTART/UNTIL are specified in the string, even with times set at 12AM UTC, is a bug on their side. I'd just like to make sure that stripping out DTSTART/DTEND and having 8/31 be returned is correct behavior from your perspective so that we can continue trying out this updated library. The main reason we want to switch is that DDay.iCal is extremely slow. We need to evaluate tens to thousands of recurrence pattern strings like this one within the scope of a user action. To alleviate this, I've had to add a hack where I can evaluate very simple recurrence patterns (daily, weekly, with BYDAY and INTERVAL and nothing else) using my own custom code and for loops, but anytime a pattern doesn't fall into the simple subset the run time goes from being basically immeasureable to upwards of 30 seconds, since it starts going into DDay.iCal. |
The pattern I'm using:
FREQ=WEEKLY;INTERVAL=1;BYDAY=WE;DTSTART=20160801T070000Z;UNTIL=20160831T070000Z
UNTIL is inclusive, unlike DTEND which is non-inclusive
iCal.NET drops 8/31 out of the date list, while DDay.iCal does not
The text was updated successfully, but these errors were encountered: