Edgewall Software
Modify

Opened 12 years ago

Closed 12 years ago

Last modified 11 years ago

#10863 closed defect (fixed)

fix to_datetime() when given timezone naive datetime input

Reported by: Christian Boos Owned by: Christian Boos
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.

Attachments (1)

t10863-to_datetime-normalized.diff (4.6 KB ) - added by Christian Boos 12 years ago.
rework to_datetime

Download all attachments as: .zip

Change History (11)

comment:1 by Christian Boos, 12 years ago

Milestone: 0.12.5

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

comment:2 by Christian Boos, 12 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, 12 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, 12 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, 12 years ago

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

comment:6 by Christian Boos, 12 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, 12 years ago

rework to_datetime

comment:7 by Jun Omae, 12 years ago

I tried the latest patch and it works good!

comment:8 by Christian Boos, 12 years ago

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

Applied in r11397. Thanks for the review!

comment:9 by Genie, 12 years ago

Keywords: pytz timezone added

comment:10 by Christian Boos, 11 years ago

Owner: set to Christian Boos

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Christian Boos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Christian Boos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.