-
Notifications
You must be signed in to change notification settings - Fork 523
Add timezone to generated DTSTART when UNTIL is aware #693
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
Conversation
|
|
||
| from freezegun import freeze_time | ||
|
|
||
| import pytest |
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 isn't used.
dateutil/test/test_rrule.py
Outdated
|
|
||
|
|
||
| @freeze_time(datetime(2018,3,6,5,36,tzinfo=tz.tzutc())) | ||
| def testDtstartTimeZone(self): |
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 are some PEP 8 violations in this test. Arguments need spaces between them, e.g.
datetime(2018, 3, 6, 5, 36, tzinfo=tz.tzutc()) and rrule(freq=HOURLY, until=datetime(2018, 3, 6, 8, 0, ...)
dateutil/test/test_rrule.py
Outdated
|
|
||
| @freeze_time(datetime(2018,3,6,5,36,tzinfo=tz.tzutc())) | ||
| def testDtstartTimeZone(self): | ||
| rule_without_dtstart = rrule(freq=HOURLY,until=datetime(2018,3,6,8,0,tzinfo=tz.tzutc())) |
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 would prefer tz.UTC be used instead of tz.tzutc().
dateutil/test/test_rrule.py
Outdated
| def testDtstartTimeZone(self): | ||
| rule_without_dtstart = rrule(freq=HOURLY,until=datetime(2018,3,6,8,0,tzinfo=tz.tzutc())) | ||
| rule_with_dtstart = rrule(freq=HOURLY,dtstart=datetime(2018,3,6,5,36,tzinfo=tz.tzutc()),until=datetime(2018,3,6,8,0,tzinfo=tz.tzutc())) | ||
| assert list(rule_without_dtstart) == list(rule_with_dtstart) |
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.
You can leave this as is, but generally in the unittest-style tests, you use self.assertEqual instead of bare assert statements. In this case I'm just going to convert this over to a pytest style test before merging, though, so you can leave this as is.
See gh issue #652 - before this the generated DTSTART was always naive, even if UNTIL had a time zone, leading to error cases.
|
@absreim I did the cleanup myself on this one because I figured I would want to do the unittest -> pytest conversion anyway. Thanks for your contribution! It's obvious you put a lot of thought into this! |
Summary of changes
Adds a feature when initializing an rrule with a specified UNTIL but without an explicitly specified DTSTART so that the generated DTSTART takes on the time zone of the UNTIL. Without this feature in that situation, rrule would raise a ValueError because the generated DTSTART is not time zone aware.
Closes #652
Pull Request Checklist