-
Notifications
You must be signed in to change notification settings - Fork 33
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
Support localized cronjob scheduling with ambiguous and gap DateTime instances #133
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this great PR. ❤️
I especially like the handling of the cron expression sigil syntax.
I had a first detailed look and left some comments.
I'll try to represent a few more special cases as tests and check if they behave as expected.
cond do | ||
?a in options and ?l in options -> [:earlier, :later] | ||
?a in options -> [:earlier] | ||
true -> [:later] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we should have the option to not run at all in cases of ambiguity. How would that be expressed?
|
||
def parse("@" <> identifier, _) do | ||
def parse("@" <> identifier, _, _) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we also need to pass the ambiguity options to specials. For example most of them run at midnight. There's no guarantee that a TZ change does not happen over midnight.
end | ||
|
||
def resolve_potential_gap(%{std_offset: n}, candidate = %{std_offset: m}, amt, _) do | ||
cond do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs a bit more detail since DST shifts are not guaranteed to be one hour.
For example, Lord Howe Island is UTC +10:30 / +11:00
We also need to add the ambiguity options into |
@shaolang I pushed addtional tests to the PR. I believe they should not fail. Can you check? |
{"30 2 * * *", [], "Europe/Zurich", ~N[2024-03-31 01:00:01], ["2024-04-01T02:30:00+02:00"]}, | ||
|
||
# Half Hour Shift | ||
{"*/30", [:earlier], "Australia/Lord_Howe", ~N[2024-04-07 01:15:01], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should actually fail; the cron expression expects the frequency to be "every 30 minute", so shift/4
ignores the ambiguity opt; same comment for the other half-hour shift cases. I'll remove these test cases if you have no objections; alternatively, I can update them to the expected values, though I don't see the value-add in keeping them.
["2024-10-27T02:30:00+01:00", "2024-10-28T02:30:00+01:00"]}, | ||
{"30 2 * * *", [:earlier, :later], "Europe/Zurich", ~N[2024-10-27 01:00:01], | ||
["2024-10-27T02:30:00+02:00", "2024-10-27T02:30:00+01:00", "2024-10-28T02:30:00+01:00"]}, | ||
{"30 2 * * *", [], "Europe/Zurich", ~N[2024-10-27 01:00:01], ["2024-10-28T02:30:00+01:00"]}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shaolang I think we fist have to be clear about the Crontab.DateChecker
. The scheduler should find all dates efficiently for instances where the date checker evaluates to truthy.
I've replaced DateHelper.add/3 with DateHelper.shift/4 that:
:on_ambiguity
settings.CronExpression
gains 2 options:a
: returns earlier time when calculated DateTime is ambiguous.l
: returns later time when calculated DateTime is ambiguous.I didn't add the
b
option to cover both 'cos specifyingal
is clearer, imo.I've also replaced all
NaiveDateTime.add/3
calls inScheduler
withDateHelper.shift/4
. AsDateHelper.shift/4
is rigorously tested, I didn't add tests for the updatedScheduer
.Closes #131.