-
-
Notifications
You must be signed in to change notification settings - Fork 71
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
Fix can't compare offset-naive and offset-aware datetimes error #70
Fix can't compare offset-naive and offset-aware datetimes error #70
Conversation
Codecov Report
@@ Coverage Diff @@
## master #70 +/- ##
==========================================
- Coverage 84.86% 83.03% -1.84%
==========================================
Files 4 4
Lines 337 330 -7
Branches 79 79
==========================================
- Hits 286 274 -12
- Misses 27 31 +4
- Partials 24 25 +1
Continue to review full report at Codecov.
|
self.eventA.start = datetime(year=2017, month=2, day=3, hour=12, minute=5) | ||
self.eventA.end = datetime(year=2017, month=2, day=3, hour=15, minute=5) | ||
self.assertEqual('2017-02-03 12:05:00+00:00: Event A (ended)', str(self.eventA)) |
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 can't see how this actually tests your proposed change. You don't test direct comparison, just the string representations.
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.
The bug reported in #69 is that str(event)
fails for all-day events, which use naive datetime objects. This tests str(event)
which is where the patch is and the bug occurs. The test added here fails in master and no longer fails with this patch. The error was introduced by #66 which enforces that all_day events do not have any timezone associated with them and use naive datetimes, so no code using event.start/end may assume tz-aware datetime objects for comparison.
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.
Oh, I see now.
For some reason I missed that this was in the __str__
-method when I reviewed it the first time.
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.
any chance this will get pushed to pypi anytime soon? |
I am still waiting for access rights - sorry @SkiTheSlicer |
…set-datetimes-error Fix can't compare offset-naive and offset-aware datetimes error
Fixes Issue #69
Testing
Remove the new code in
icalevents/icalparser.py
and run tests, the new test should fail. Put the code back and the test should pass.