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.

Sign up for GitHub

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

Jump to bottom

Fix can't compare offset-naive and offset-aware datetimes error #70

Merged
merged 2 commits into from
Feb 11, 2021
Merged

Fix can't compare offset-naive and offset-aware datetimes error #70

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
31a1b27
Fix can't compare offset-naive and offset-aware datetimes error
kaitlynmcgoldrick Aug 25, 2020
bd4a63f
Merge branch 'master' into fix-compare-offset-datetimes-error
Hultner Feb 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions icalevents/icalparser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,11 @@ def __lt__(self, other):
def __str__(self):
n = now()

if not self.start.tzinfo:
self.start = normalize(self.start)
if not self.end.tzinfo:
self.end = normalize(self.end)

# compute time delta description
if not self.all_day:
if self.end > n > self.start:
Expand Down
5 changes: 5 additions & 0 deletions test/test_icalparser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,3 +100,8 @@ def test_attendee(self):
def test_organizer(self):
self.assertIsInstance(self.eventA.organizer, str)
self.assertIsInstance(self.eventB.organizer, str)

def test_str(self):
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))
Copy link
Member

@Hultner Hultner Sep 17, 2020 •
edited
Loading

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.

Copy link

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.

Copy link
Member

@Hultner Hultner Feb 11, 2021 •
edited
Loading

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pointer in the right direction @minrk.

I’ve approved and merged this now, I can unfortunately not release new versions to pypi as I don’t have the credentials so we’ll have to wait for @irgangla to get this released.