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

Add ready for use RRule instance with caching #20

Merged
merged 9 commits into from
May 6, 2021

Conversation

polRk
Copy link
Contributor

@polRk polRk commented Apr 21, 2021

Add RRule ready for use instance with result caching

Checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

@polRk
Copy link
Contributor Author

polRk commented Apr 21, 2021

I do not know how to cache after method without recurrence rule until value. Can u help me?

@polRk
Copy link
Contributor Author

polRk commented Apr 21, 2021

I'm inspired by this repository https://github.com/jakubroztocil/rrule

lib/src/rrule.dart Outdated Show resolved Hide resolved
@JonasWanke
Copy link
Owner

Thanks for this contribution! Caching seems like a really useful feature as the internal calculations in this package can get quite complex.

I have a few questions/comments/suggestions about the architecture of this caching:

  • Having a class called RRule (the common abbreviation for Recurrence Rule) next to the class RecurrenceRule seems a bit confusing. Could we either integrate this functionality directly in RecurrenceRule or maybe rename RRule to CachingRecurrenceRule?

  • rrule.js's before and after only return a single value (the first event before/after the given instant) as opposed to all values before/after that instant, but I think that the latter is useful as well. (And if we need the former in the future, we can still add methods calculating and returning only the single next event.) However, I'm not sure whether we really need three separate methods (before, after, and between). Maybe we can combine those into a single method with optional before and after parameters?

    List<DateTime> getAllInstances({
      required DateTime start, // Only necessary if this is integrated into `RecurrenceRule` directly.
      DateTime? after,
      bool includeAfter = false,
      DateTime? before,
      bool includeBefore = false,
    }) {/* ... */}
  • Currently, a hash of all relevant parameters (rrule, start, and method arguments) is used as the cache key. While hash functions try to minimize the risk of collisions, those are not impossible. Hence, I recommend creating a utility class like _CacheKey which stores those arguments directly and, as the name suggests, is used as the cache key. And as the cache is currently stored per recurrence rule, the rrule doesn't even have to be part of this key.

  • RRule accepts an initialCache, similar to rrule.js. Do you know of any use-cases for supplying that initial cache state, especially since the cache itself is not made public (and hence can't be persisted otherwise by users of this package)?

@polRk
Copy link
Contributor Author

polRk commented Apr 26, 2021

RRule accepts an initialCache, similar to rrule.js. Do you know of any use-cases for supplying that initial cache state, especially since the cache itself is not made public (and hence can't be persisted otherwise by users of this package)?

This can reduce the cold start. If I have 1000 recurring tasks, it will be better to use cached values rather than calculated ones.

@polRk polRk reopened this Apr 26, 2021
Copy link
Owner

@JonasWanke JonasWanke left a comment

Choose a reason for hiding this comment

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

This looks great! I've added some minor comments, and then this can get merged :)

(You can ignore the failing lint actions.)

lib/src/recurrence_rule.dart Outdated Show resolved Hide resolved
lib/src/cache.dart Outdated Show resolved Hide resolved
lib/src/cache.dart Outdated Show resolved Hide resolved
lib/src/cache.dart Outdated Show resolved Hide resolved
test/cache_test.dart Outdated Show resolved Hide resolved
polRk and others added 5 commits May 3, 2021 14:35
Co-authored-by: Jonas Wanke <contact@wanke.dev>
Co-authored-by: Jonas Wanke <contact@wanke.dev>
Co-authored-by: Jonas Wanke <contact@wanke.dev>
Co-authored-by: Jonas Wanke <contact@wanke.dev>
@JonasWanke JonasWanke merged commit 86945d9 into JonasWanke:main May 6, 2021
@JonasWanke
Copy link
Owner

This is now published as part of v0.2.3. Thanks for contributing!

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