impl DoubleEndedIterator for ScheduleIterator #66
Conversation
This implements DoubleEndedIterator for ScheduleIterator in order to get previous run times.
|
This is excellent! I always wanted an implementation of this but never got around to writing it. Thanks for putting it together! I agree that this will likely require reconsidering some method names, but we can take care of that in a separate PR. It'll be a day or two before I have time to read through the diff properly. Could you make sure that there's a test for iterating backwards over a schedule that includes non-existent times? (e.g. Hours that are skipped due to daylight saving time.) Here's the corresponding forward-iteration unit test. |
|
@zslayton I have added the test for non-existent times and fixed a bug causing it to misreport. let me know if there's anything you need. |
|
I read through it this morning and it looks great. Thanks! Regarding naming, I think I'd like // Iterates forwards
schedule.after(datetime)
// Iterates backwards
schedule.before(datetime)
// Double-ended iterator
schedule.range(start, end)
// Equivalent to schedule.after(Tz.now())
schedule.upcoming(<
8000
span class="pl-v">Tz)
// Equivalent to schedule.before(Tz.now())
schedule.history(Tz)I think this makes the iterator behavior associated with each method more obvious. In particular, you only get a double-ended iterator a range that, well, has two ends. I'm not married to any of those names, but that's the gist. What're your thoughts? |
|
I was thinking of perhaps simplifying the API to something like: // Returns an iterator without specifying direction, but like most forward by default but can use .rev() to go backwards.
schedule.using(datetime)This keeps the API surface area tiny and simple. Here are my thoughts on the proposed: schedule.after(datetime)
^ implies we can only go forward, which isn't true of the iterator
schedule.before(datetime)
^ implies we can only go backward, which isn't true of the iterator
schedule.range(start, end)
^ not sure why we would need a range as the CRON itself specifies a range or you can limit using .take(n)
schedule.upcoming(Tz)
schedule.history(Tz)
^^ both of these don't seem to be necessary since using a date time or Tz you need chrono as a dependancy in the calling program already. Why not just let people create the date time themselves and eliminate 2 more methods and shrink the API surface.In a bit of a rush this morning so will give above even more thought later tonight. Hope this doesn't seem too drastic. |
Now that we have a complete implementation of both forward and backward iteration, it would be trivial to return a single-direction iterator. My thinking is that there aren't common use cases for switching directions mid-stream (i.e. iterate over several datetimes, then call
The cron schedule itself can include a range, but it often doesn't. For example,
I agree that this is functionally equivalent, but I find: schedule.using(datetime).rev()to be much less self-explanatory than: schedule.before(datetime)Maybe there's a clearer name than
I think you're asking why the crate doesn't just require users to do: schedule.after(Tz.now())Is that right? If so, I agree that this is a minor convenience.
Keeping a library's API surface small is wise, but in this case I'm not concerned about the library going from 2 methods to ~5. Most of them would just be convenience methods provided in the interest of code readability. |
|
@zslayton all makes sense to me. Ultimately it's your library and choice so take everything I say/said with a grain of salt :) I just like keeping the surface as small as possible until I know people actually want/need the convenience abstractions. To me using I agree In any case I think your proposed API is good and think below should still be removed: schedule.upcoming(Tz)
schedule.history(Tz)love the library, learning a lot and may take a stab at a CRON parser myself to learn some more too :) |
I definitely see the value this approach! I'll try to think of some version of
Yeah, I think the case for these is the weakest.
Thanks! Do check out nom v6+ (reddit discussion), it replaced the macros this library uses (from nom ~4) with a really clever application of generics. I never have as much project time as I'd like, but I've been considering updating Thanks again for this (major!) contribution. It's great to finally have this feature. |
|
BTW love |
This PR implements
DoubleEndedIteratorforScheduleIteratorin order to retrieve previous scheduled times.My use case is I'm building a CRON scheduler using this crate where each run of a scheduled job will save the last time it was successfully run. This saved time will be used by a service using this scheduler to know on startup, either due to scheduled maintenance, downtime or otherwise, If a scheduled job was missed being run during that downtime and should be run immediately to make up for it.
So this will allow checking the previous scheduled run time against the last time it was run in order to make that determination.
P.S. The function name
upcomingused to get aScheduleIteratordoes read strange when iterating in reverse over schedule times and perhaps should change with a major version change, but until that time didn't want to make breaking changes.