JonasWanke / rrule

🔁 Recurrence rule parsing & calculation as defined in the iCalendar RFC
https://pub.dev/packages/rrule
Apache License 2.0
52 stars 22 forks source link

feat(iteration): add shortcut for iterating over dates #66

Closed jonasbadstuebner closed 9 months ago

jonasbadstuebner commented 9 months ago

I couldn't stop thinking about what is going to happen if the calendar package I'm currently working on is going to still be used in the far future.

I was wondering, how to solve the issue of finding the recurrences of an event I create as infinitely recurring from next Friday, if it is never stopped and 900 or so years have passed. (900 just as in "many")

Given the following code:

const testRrule = 'RRULE:FREQ=DAILY;INTERVAL=2';

final recurrencesFarFromNow = RecurrenceRule.fromString(testRrule)
    .getInstances(
      start: DateTime.utc(2000),
      after: DateTime.utc(2900),
      before: DateTime.utc(2901),
    )
    .toList();

It would iterate over 900 years of recurrence, in total over 160.000 DateTimes that get compared and dropped, because they are not in range yet.

My code change is quite small, but improves this behavior significantly. It allows to "look back" at past recurrences chunk-wise, instead of calculating and caching them. This was impossible before, because the calculations were just too inefficient to always skip all of these years for every new chunk to load.

All tests are passing, I would still request you, @JonasWanke to think with me about a case where this small calculation would fail. Maybe I also missed something.

codecov[bot] commented 9 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (0c1a581) 87.76% compared to head (ca0a4de) 87.90%. Report is 4 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #66 +/- ## ========================================== + Coverage 87.76% 87.90% +0.13% ========================================== Files 21 21 Lines 1275 1281 +6 ========================================== + Hits 1119 1126 +7 + Misses 156 155 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jonasbadstuebner commented 9 months ago

I also had the feeling it would be more complicated... And I am still not sure, if it is really this easy...I feel like there would be an edge case, where this would break. But I couldn't think of one and if you also couldn't find a problem, I guess we have to wait until someone complains in another issue. :smile:

jonasbadstuebner commented 9 months ago

Are you sure about the bySetPositions working correctly?

jonasbadstuebner commented 9 months ago

I found some weird behavior. But it is not related to my change, as it also happens without it. I will create a new issue or PR for this.

I am now convinced this PR is safe to merge. :tada:

JonasWanke commented 9 months ago

Thanks for this performance improvement! It's published as part of v0.2.15

JonasWanke commented 9 months ago

Well, someone found the edge case: #71. The problem isn't bySetPositions, but interval > 1. For now, I've limited the optimization to only apply with an interval of one: https://github.com/JonasWanke/rrule/commit/902b0716086e7cf7bc4fea2f25c55ea8af41e340.

We can probably handle other intervals as well by not jumping ahead to after directly, but rather to start plus a multiple of frequency * interval, so that we're still before after. But I'll wait until the migration to Chrono before implementing that

jonasbadstuebner commented 9 months ago

Ah, makes sense. I think I might come back to this and implement an idea for interval, now that the/one edge case is clear.