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

Rework DST handling #34

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Rework DST handling #34

wants to merge 1 commit into from

Conversation

kiorky
Copy link
Owner

@kiorky kiorky commented Oct 12, 2020

rel #32 & #23 & taichino#137

@Dunedan
Copy link

Dunedan commented Oct 12, 2020

I just checked this branch to see if it fixes taichino#147. Turns out it doesn't:

from datetime import datetime

from croniter import croniter_range
from dateutil.tz import gettz

for i in croniter_range(
        datetime(2021, 3, 26, tzinfo=gettz("Europe/Berlin")),
        datetime(2021, 3, 31, tzinfo=gettz("Europe/Berlin")),
        "55 12 * * *"
):
    print(i)
2021-03-26 12:55:00+01:00
2021-03-27 12:55:00+01:00
2021-03-28 13:00:00+02:00
2021-03-29 12:55:00+02:00
2021-03-30 12:55:00+02:00

The code from this PR is also much slower than before.

I believe the best approach would be to not try to manually calculate the DST-related changes, but to let Python's datetime module handle that itself, by consequently using datetime objects internally and only convert to timestamps whenever the user wants to get timestamps.

Here is an example of Python handling that (of course without the cron-expression related logic):

from datetime import datetime, timedelta

from dateutil.tz import gettz

dt = datetime(2021, 3, 25, 12, 55, tzinfo=gettz("Europe/Berlin"))
for _ in range(0, 5):
    dt = dt + timedelta(days=1)
    print(dt)
2021-03-26 12:55:00+01:00
2021-03-27 12:55:00+01:00
2021-03-28 12:55:00+02:00
2021-03-29 12:55:00+02:00
2021-03-30 12:55:00+02:00

@kiorky
Copy link
Owner Author

kiorky commented Oct 13, 2020

wait for the PR to complete, as i already said, for now its just a WIP to discuss with @lowell80, and no, it's not possible to use datetime everywhere without rewriting croniter entirely.

For the speed regression, it's a tradeoff we have to put if we want to handle DST transition.

@kiorky kiorky marked this pull request as draft October 13, 2020 10:44
@lowell80
Copy link

I looked over the code you have so far. Unfortunately, I was trying to get my head wrapped around the core logic of it all, but it's a bit out of reach at the moment. I think I'd need to spend several consecutive hours staring at it, or more. :-)

Given that the next DST transition is Nov 1st (at least here anyway), I'm guessing there won't be anything ready for
"prod-run" by then?

I can certainly help with testing when you are ready for that phase. I have a library of user-generated CRON expressions on hand! :-) Just give a shout!

@kiorky
Copy link
Owner Author

kiorky commented Oct 15, 2020

Yep, and the period in France is a bit not "safe" for me right now to be as productive as i would want...

Idea is to stick with the unix strategy where that window with a blackhole exists, jobs are scheduled next hour.

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.

3 participants