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

Correctly handle DST changes #92

Merged
merged 3 commits into from
Nov 6, 2017
Merged

Correctly handle DST changes #92

merged 3 commits into from
Nov 6, 2017

Conversation

kbrose
Copy link
Contributor

@kbrose kbrose commented Nov 6, 2017

See #91

There are two main changes:

  • Keep track of the most recently seen dst offset to avoid infinite loops.
  • Fix incorrect arithmetic around DST offsets (before the offset was either added or subtracted depending on its sign, but this should not have happened).

You can verify it by running

import datetime as dt
import pytz
import croniter

s = dt.datetime.strptime('2017-11-01', '%Y-%m-%d')
s = pytz.timezone('US/Eastern').localize(s)
ci = croniter.croniter('0 16 * * *', s)

for i in range(8):
    print(dt.datetime.utcfromtimestamp(next(ci)))

print('-'*19)

s = dt.datetime.strptime('2017-03-08', '%Y-%m-%d')
s = pytz.timezone('US/Eastern').localize(s)
ci = croniter.croniter('0 16 * * *', s)

for i in range(8):
    print(dt.datetime.utcfromtimestamp(next(ci)))

expected output:

2017-11-01 20:00:00
2017-11-02 20:00:00
2017-11-03 20:00:00
2017-11-04 20:00:00
2017-11-05 21:00:00
2017-11-06 21:00:00
2017-11-07 21:00:00
2017-11-08 21:00:00
-------------------
2017-03-08 21:00:00
2017-03-09 21:00:00
2017-03-10 21:00:00
2017-03-11 21:00:00
2017-03-12 20:00:00
2017-03-13 20:00:00
2017-03-14 20:00:00
2017-03-15 20:00:00

The tests on master are currently broken (ref #89), with test_std_dst failing. With this change, that test now passes, but unfortunately test_std_dst2 fails. However, I am not sure that test_std_dst2 is correctly written. It seems like that test expects the cron string to be specifying the UTC trigger times, but in reality the cron string is interpreted as the local timezone. I'm tempted to rewrite/delete that test. I would like someone else's opinion on that, though, as I'm not 100% sure.

@kbrose
Copy link
Contributor Author

kbrose commented Nov 6, 2017

Looking more at the original issue that spawned test_std_dst2 (#87), I'm not convinced it was ever fixed. The changes in this PR appear to be behaving correctly in the test case given by OP of #87:

from __future__ import print_function
import pytz
from datetime import datetime, timedelta
from croniter import croniter

tz = pytz.timezone("America/Sao_Paulo")

local_dates = [tz.localize(datetime(2018, 2, 17, 0, 0, 0)),
               tz.localize(datetime(2018, 2, 17, 1, 0, 0)),
               tz.localize(datetime(2018, 2, 17, 2, 0, 0)),
               tz.localize(datetime(2018, 2, 17, 3, 0, 0))]

[print(d, '=>', croniter("0 0 * * *", d).get_next(datetime)) for d in local_dates]
print('-' * 30)
local_dates_5min = [d + timedelta(minutes=5) for d in local_dates]
[print(d, '=>', croniter("0 0 * * *", d).get_next(datetime)) for d in local_dates_5min]
2018-02-17 00:00:00-02:00 => 2018-02-18 00:00:00-03:00
2018-02-17 01:00:00-02:00 => 2018-02-18 00:00:00-03:00
2018-02-17 02:00:00-02:00 => 2018-02-18 00:00:00-03:00
2018-02-17 03:00:00-02:00 => 2018-02-18 00:00:00-03:00
------------------------------
2018-02-17 00:05:00-02:00 => 2018-02-18 00:00:00-03:00
2018-02-17 01:05:00-02:00 => 2018-02-18 00:00:00-03:00
2018-02-17 02:05:00-02:00 => 2018-02-18 00:00:00-03:00
2018-02-17 03:05:00-02:00 => 2018-02-18 00:00:00-03:00

@kiorky
Copy link
Collaborator

kiorky commented Nov 6, 2017

I validate the changes, its released, thx !

@kiorky kiorky merged commit 5917852 into taichino:master Nov 6, 2017
@kiorky
Copy link
Collaborator

kiorky commented Nov 6, 2017

@kbrose kbrose deleted the dst branch November 6, 2017 21:40
@kbrose
Copy link
Contributor Author

kbrose commented Nov 6, 2017

Thanks @kiorky !

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.

2 participants