consistent use of tzinfo.normalize()

[Christian Boos]

As a follow-up to #10863, I had a closer look to our use of tz.normalize().

According to ​http://pytz.sourceforge.net/#localized-times-and-date-arithmetic, using normalize() is necessary after having performed arithmetic on localized datetime, and when switching timezones (t.astimezone(tz), except when tz is utc).

There are several places in trac/util/datefmt.py where we use astimezone() or do arithmetic without wrapping this in normalize().

There's also a place in to_datetime() where we use normalize(localize()), which seems unnecessary.

Beyond datefmt.py, there are other places where we do arithmetic on localized dates, we should handle them as well when the end result will likely end up being displayed (e.g. a milestone's duedate). It could be left unchanged when the computed datetime will be used for its "absolute" value (e.g. start in the TimelineModule).

[Christian Boos]

adding calls to normalize where I think we should call it, removing it when it shouldn't be needed

[Christian Boos]

As for #10863, I think this could be OK for 0.12-stable but I'd like to get some reviewing first!

The patch is for trunk and lacks extra tests for now.

---

In the meantime, I saw jomae comment on #10863, so I'll adapt the patch shortly…

[Christian Boos]

second take, now only adding some calls to normalize

[Jun Omae]

I confirmed the patch. If accoss a DST boundary, it would suggest 17:00 or 19:00 in trac/ticket/roadmap.py. Using datetime.replace makes similar problems, e.g. _parse_relative_time in trac/util/datefmt.py.

My suggested patch to use to_datetime instead of datetime.replace is based on r11388 with t10863-to_datetime-normalized.diff​.

trac/ticket/roadmap.py

@@ -33 +33 @@
 from trac.util import as_bool
 from trac.util.datefmt import parse_date, utc, to_utimestamp, \
                               get_datetime_format_hint, format_date, \
-                              format_datetime, from_utimestamp, user_time
+                              format_datetime, from_utimestamp, user_time, \
+                              to_datetime
 from trac.util.text import CRLF
 from trac.util.translation import _, tag_
 from trac.ticket import Milestone, Ticket, TicketSystem, group_milestones
@@ -793 +794 @@
 
     def _render_editor(self, req, milestone):
         # Suggest a default due time of 18:00 in the user's timezone
-        default_due = datetime.now(req.tz).replace(hour=18, minute=0, second=0,
-                                                   microsecond=0)
-        if default_due <= datetime.now(utc):
-            default_due += timedelta(days=1)
-
+        now = datetime.now(req.tz)
+        default_due = to_datetime(datetime(now.year, now.month, now.day, 18),
+                                  tzinfo=req.tz)
+        if default_due <= now:
+            default_due = req.tz.normalize(default_due + timedelta(days=1))
+            if default_due.hour != 18: # if across a DST boundary
+                default_due += timedelta(hours=18 - default_due.hour)
+
         data = {
             'milestone': milestone,
             'datetime_hint': get_datetime_format_hint(req.lc_time),

[Christian Boos]

Shouldn't the last line be more something like:

default_due = to_datetime(default_due.replace(hours=18))

i.e. have a normalize step after doing arithmetics with timedelta or like here, a .replace() (the latter should achieve the same effect, but more directly).

Also for _parse_relative_time, it's only used in parse_date which does call to_datetime() on the returned value of _parse_relative_time, so I think it's OK.

[Jun Omae]

Replying to cboos:

Also for _parse_relative_time, it's only used in parse_date which does call to_datetime() on the returned value of _parse_relative_time, so I think it's OK.

Sure. But we don't use the returned value of to_datetime….

And, the following code introduced in r6318 does not work at trunk/trac/util/datefmt.py@11332:479-486#L451​.

    # Make sure we can convert it to a timestamp and back - fromtimestamp()
    # may raise ValueError if larger than platform C localtime() or gmtime()
    try:
        to_datetime(to_timestamp(dt), tzinfo)
    except ValueError:

>>> from trac.util.datefmt import parse_date, to_timestamp, to_datetime, timezone
>>> t = parse_date('2100-01-01T01:01:01Z')
>>> t
datetime.datetime(2100, 1, 1, 1, 1, 1, tzinfo=<FixedOffset "UTC" 0:00:00>)
>>> to_timestamp(t)
4102448461L
>>> to_datetime(to_timestamp(t), timezone('Europe/Paris'))
datetime.datetime(1970, 1, 1, 2, 8, 22, 448461, tzinfo=<DstTzInfo 'Europe/Paris' CET+1:00:00 STD>)

[Christian Boos]

Replying to jomae:

And, the following code introduced in r6318 does not work at trunk/trac/util/datefmt.py@11332:479-486#L451​.

This seems related to r9789: the values for _min_ts and _max_ts seem rather arbitrary there, as those values are rather useful for avoiding to raise the ValueError when using datetime.fromtimestamp() in the first place.

The boundary values we should check before attempting to "convert" from usec to sec should rather be to_timestamp(to_datetime(datetime.{min,max})).

trac/util/datefmt.py

@@ -61 +61 @@
     elif isinstance(t, date):
         dt = tz.localize(datetime(t.year, t.month, t.day))
     elif isinstance(t, (int, long, float)):
-        if not (_min_ts <= t <= _max_ts):
+        if not (_min_ts_dt <= t <= _max_ts_dt):
             # Handle microsecond timestamps for 0.11 compatibility
             t *= 0.000001
         if t < 0 and isinstance(t, float):
@@ -444 +444 @@
 _zero = timedelta(0)
 _min_ts = -(1 << 31)
 _max_ts = (1 << 31) - 1
+_min_ts_dt = -62135600400L # to_timestamp(to_datetime(datetime.min))
+_max_ts_dt = 253402297199L # to_timestamp(to_datetime(datetime.max))
 
 localtz = LocalTimezone()

>>> from trac.util.datefmt import *
>>> t = parse_date('2100-01-01T01:01:01Z')
Traceback (most recent call last):
  File "<stdin>", line 1, in ?
  File "trac\util\datefmt.py", line 282, in parse_date
    _('Invalid Date'))
trac.core.TracError: The date "2100-01-01T01:01:01Z" is outside valid range. Try a date closer to present time.

FIXED (see comment:12)

[Jun Omae]

I worked in repos:jomae.git:ticket10864/0.12, fixed some defects and added some unit tests for datefmt.*. Would somebody please review it?

[Christian Boos]

Still got one failure with [4ab8ae8df/jomae.git], on Windows 7, Python 2.7.2:

FAIL: test_max_timestamp (trac.util.tests.datefmt.ParseDateValidRangeTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "c:\Trac\repos\0.12-stable\trac\util\tests\datefmt.py", line 455, in test_max_timestamp
    '2038-01-19T03:14:08Z')
AssertionError: TracError not raised

Suggesting something like:

trac/util/tests/datefmt.py

@@ -459 +460 @@
         self.assertRaises(TracError, datefmt.parse_date,
                           '1901-12-13T20:45:51Z')
 
-    if os.name == 'nt': # negative timestamps not supported on Windows
-        del test_min_timestamp
+    if os.name == 'nt':
+        if sys.version_info < (2, 6):
+            del test_min_timestamp
+        else:
+            # maximal timestamp is much higher on Windows after 2.6
+            def test_max_timestamp(self):
+                datefmt.parse_date('3001-01-01T20:59:59Z')
+                self.assertRaises(TracError, datefmt.parse_date,
+                                  '3001-01-01T21:00:00Z')
+
+            # negative timestamps supported a tiny bit on Windows after 2.6
+            def test_min_timestamp(self):
+                datefmt.parse_date('1969-12-31T12:00:00Z')
+                self.assertRaises(TracError, datefmt.parse_date,
+                                  '1969-12-31T11:59:59Z')
 
 
 class DateFormatTestCase(unittest.TestCase):

(see [2240fdf2/cboos.git])

The above works with 2.4.4, 2.5.4, 2.6.5, and 2.7.2. I have no idea if the change happened for 2.6 strict or in versions between 2.5.4 and 2.6.5…

[Christian Boos]

Another remark: what about using a bit of "Hungarian notation" for the datetime objects in timeline/web_ui.py and ticket/roadmap.py? It could make the usage clearer if we track the nature of the datetime object in the name (e.g. naive_yesterday instead of just yesterday).

Note that I'm not suggesting to do that in datefmt, as there we are supposed to be careful and know what we're doing. Elsewhere, it would remind there's something special going on.

[Christian Boos]

Replying to cboos:

… (see [2240fdf2/cboos.git])

And that's not even enough: on Linux x86_64, Python 2.7.3, the assertions are not raised either.

[Christian Boos]

I confirm that with [b3524a99/jomae.git], the tests pass on my platforms (Python 2.5.4, 2.6.5, 2.7.2 (32-bits) and Python 2.7.2 (64-bits) on Windows 7 (x64), Python 2.7.3 64-bits on Linux x86_64).

What about comment:5 and comment:9? And are you planning more changes? Otherwise we could start with applying what you have so far here, refining if necessary and then merge #10912 (but you may have a better plan, so I let you "drive" in the end ;-) ).

[Jun Omae]

Replying to cboos:

I confirm that with [b3524a99/jomae.git], the tests pass on my platforms (Python 2.5.4, 2.6.5, 2.7.2 (32-bits) and Python 2.7.2 (64-bits) on Windows 7 (x64), Python 2.7.3 64-bits on Linux x86_64).

Thanks for your confirmations! I've tested with my environments:

• Python 2.7.1 (64-bits) on Ubuntu 12.10
• Python 2.5.5 and 2.6.6 (64-bits) on Debian 6.0.6
• Python 2.4.3, 2.5.6, 2.6.8 and 2.7.2 (32-bits) on CentOS 5.8
• Python 2.7.2 (64-bits) on FreeBSD 8.3
• Python 2.4.4, 2.5.4, 2.6.6 and 2.7.1 (32-bits) on Windows XP sp3

What about comment:5 and comment:9? And are you planning more changes?

No, I'm not planning.

About comment:5, using datetime.utcfromtimestamp instead of to_datetime fixes the issue in comment:4.

It could make the usage clearer if we track the nature of the datetime object in the name (e.g. naive_yesterday instead of just yesterday).

yesterday is an aware datetime object….

(but you may have a better plan, so I let you "drive" in the end ;-) ).

Many good suggestions to me. Thanks ;-)

[Jun Omae]

Applied in [11415-11417]!

[Remy Blank]

Thanks Jun for all these fixes! Who would have thought that time was so complicated ;)