Closed shaolang closed 6 months ago
Thanks a lot for the PR!
This is just the feedback for a quick glance over the code. I'll do a more thorough review later.
A cron expression, which runs every 5 minutes should also do so when DST changes.
A cron expression running at 2:30 AM every day however should not.
I'm thinking if it would therefore make sense to introduce an option argument where you could choose both, first or last. (Naming tbd)
I've removed private as a dependency and reinstated the date type in DateChecker
.
Hmmm... ambiguous times are really a headache. I chose to return one of the instances is due to AWS EventBridge Scheduler's behavior. AWS' scheduer skips running when there's a gap, and runs only once when there's ambiguity.
Nevertheless, you let me know how you'll like to proceed from here. We are getting closer to closing #69 and improving all crontab dependents :)
Hmmm... ambiguous times are really a headache. I chose to return one of the instances is due to AWS EventBridge Scheduler's behavior. AWS' scheduer skips running when there's a gap, and runs only once when there's ambiguity.
I think we should let the user decide.
What do you think about adding
on_ambiguous_time_trigger
to theCronExpression
struct? The values would beboth
,earlier
,later
and default tolater
.Like this the user can decide how he wants to handle this edge-case.
We would then pass it through as an option in a keyword list to the
DateHelper
.
Sounds good to me.
Question then is: does that mean most functions in DateHelper
need to return either one or two dates?
does that mean most functions in DateHelper need to return either one or two dates?
Probably not:
If you have the setting on both, we can just return the first occurrence. When we look for the next candidate date, we should automatically reach the later occurrence.
So the logic could be something like this:
earlier
: return earlierlater
: return laterboth
: return earlier or later depending on adjusted
I've addressed the issues in the review. I've also added targeted tests for inc_year/1
and dec_year/1
to ensure the edge cases are covered.
I've not implement the ambiguous time handling yet, 'cos that depends on changes to CronExpression
.
@shaolang Do I understand correct that you want to send a secod PR for the ambiguity handling?
@shaolang Do I understand correct that you want to send a secod PR for the ambiguity habdling?
Yes. Are you closing this PR prior to that? I'm okay with keep this or submitting a new PR.
Separate work for me. But I‘ll merge directly onto main
. We‘ll just do PR 2 before I cut a release.
Amazing work, thanks a lot for your efforts!
@maennchen I've simplified
date_helper
with an extractedadd/3
function. As focus switches to testing that new function rigorously, I have to make it public during tests, I used PragDave's private library to keepadd/3
private in non-test environments,Functions in
date_helper
are now all able to do datetime arithmetic correctly, even for time gaps when daylight savings starts, and ambiguous times when daylight savings ends.One thing to note is that for ambiguous times when daylight saving ends, I've chosen to return the time with the standard timezone, e.g., between Eastern Daylight Time (EDT) and Eastern Standard Time (EST),
add/3
returns the time with EST. If you think it should return EDT instead, let me know, and I'll change it to that.Resolves #69