Edgewall Software

Opened 10 years ago

Closed 10 years ago

Last modified 9 years ago

#10863 closed defect (fixed)

fix to_datetime() when given timezone naive datetime input — at Version 8

Reported by: Christian Boos Owned by:
Priority: normal Milestone: 0.12.5
Component: general Version: 0.12-stable
Severity: normal Keywords: datetime pytz timezone
Cc: Branch:
Release Notes:
API Changes:

to_datetime now always return timezone aware datetime objects, which are "normalized" in the pytz sense.

Internal Changes:

Description

While looking at some specifics of the trac.util.datefmt.to_datetime function, I discovered that a particular usage pattern is problematic with pytz timezones:

>>> from trac.util.datefmt import *
>>> from datetime import datetime
>>> t = to_datetime(datetime(2012,1,1), timezone("Europe/Paris"))
>>> t.utcoffset()
datetime.timedelta(0, 540)

(from gmessage:trac-dev:V0HeDyfa_pk/5r33UatF3nEJ)

The utcoffset() plays a role when converting a time to the number of ms (to_utimestamp()). Here, it has a value of 9 minutes, which is quite wrong for our purpose.

The workaround is to always specify a timezone aware datetime object to to_datetime, but this is not documented, and shouldn't be necessary either (assuming a timezone naive datetime should actually be the same as a datetime in utc).

The following should

  • trac/util/datefmt.py

    diff --git a/trac/util/datefmt.py b/trac/util/datefmt.py
    index b59a276..d88d6b1 100644
    a b def to_datetime(t, tzinfo=None):  
    6666    elif isinstance(t, datetime):
    6767        if tzinfo is not None:
    6868            if t.tzinfo is None:
    69                 t = t.replace(tzinfo=tzinfo)
     69                t = tzinfo.localize(t)
    7070            else:
    7171                t = tzinfo.normalize(t.astimezone(tzinfo))
    7272        return t

With this change, the t.utcoffset() for the example above would give 3600 seconds, i.e. the one hour we would expect.

Change History (9)

comment:1 by Christian Boos, 10 years ago

Milestone: 0.12.5

I think it's OK for 0.12-stable. Feedback welcome!

comment:2 by Christian Boos, 10 years ago

To be complete, it seems that datetime.fromtimestamp() doesn't have the problem that the datetime constructor has:

>>> datetime.fromtimestamp(1325376000, timezone("Europe/Paris")).utcoffset()
datetime.timedelta(0, 3600)

So we don't need to change the other parts of to_datetime().

comment:3 by Christian Boos, 10 years ago

Same for datetime.now(), it's OK as well:

>>> datetime.now(timezone("Europe/Paris")).utcoffset()
datetime.timedelta(0, 7200)

(see also r8857)

comment:4 by Jun Omae, 10 years ago

It looks good except datetime between DST boundaries. tzinfo.localize() generates invalid datetime between DST boundaries. I think we should use tzinfo.normalize().

>>> tz = timezone('Europe/Paris')
>>> t = datetime(2012, 3, 25, 2, 15)
>>> tz.localize(t)
datetime.datetime(2012, 3, 25, 2, 15, tzinfo=<DstTzInfo 'Europe/Paris' CET+1:00:00 STD>)
>>> tz.localize(t).utcoffset()
datetime.timedelta(0, 3600)
>>> tz.normalize(tz.localize(t))
datetime.datetime(2012, 3, 25, 3, 15, tzinfo=<DstTzInfo 'Europe/Paris' CEST+2:00:00 DST>)
>>> tz.normalize(tz.localize(t)).utcoffset()
datetime.timedelta(0, 7200)
  • trac/util/datefmt.py

    diff --git a/trac/util/datefmt.py b/trac/util/datefmt.py
    index b59a276..0fd4aa8 100644
    a b def to_datetime(t, tzinfo=None):  
    6666    elif isinstance(t, datetime):
    6767        if tzinfo is not None:
    6868            if t.tzinfo is None:
    69                 t = t.replace(tzinfo=tzinfo)
     69                t = tzinfo.localize(t)
     70                t = tzinfo.normalize(t)
    7071            else:
    7172                t = tzinfo.normalize(t.astimezone(tzinfo))
    7273        return t

comment:5 by Christian Boos, 10 years ago

Ah, yes! Thanks for the example, I'll integrate it into an unit-test.

comment:6 by Christian Boos, 10 years ago

I've tried a more "radical" approach, by making to_datetime(t, tz) always:

  • map the given input time to the given timezone (or localtz if no tz is specified)
  • return a normalized datetime (also going in the direction of #10864)

I think it's a more consistent API and it's easier to use and to remember what it does. All tests still pass.

by Christian Boos, 10 years ago

rework to_datetime

comment:7 by Jun Omae, 10 years ago

I tried the latest patch and it works good!

comment:8 by Christian Boos, 10 years ago

API Changes: modified (diff)
Resolution: fixed
Status: newclosed
Version: 0.12-stable

Applied in r11397. Thanks for the review!

Note: See TracTickets for help on using tickets.