Date helper datetime support

[shaolang]

@maennchen I've simplified date_helper with an extracted add/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 keep add/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

[coveralls]

coverage: 96.839% (+0.1%) from 96.736%
when pulling 733cc1b on shaolang:date_helper_datetime_support
into 9122b11 on maennchen:main.

[maennchen]

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.

• I would like to avoid adding an extra dependency for private functions. Specifying @moduledoc / @doc false should ve sufficient.
• Since the DateHelper module is not part of the public API, we should also not define types there. (Doc link will not work)
• I think triggering twice for ambigous datetimes would be technically correct. But it could make sense making the behavior configurable.

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)

[shaolang]

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 :)

[maennchen]

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 the CronExpression struct? The values would be both, earlier, later and default to later.

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.

[shaolang]

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 the CronExpression struct? The values would be both, earlier, later and default to later.

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?

[maennchen]

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:

• if result ambiguous and setting is earlier: return earlier
• if result ambiguous and setting is later: return later
• if result ambiguous and setting is both: return earlier or later depending on adjusted

[shaolang]

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.

[maennchen]

@shaolang Do I understand correct that you want to send a secod PR for the ambiguity handling?

[shaolang]

@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.

[maennchen]

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!