[Patches] [ python-Patches-1667546 ] Time zone-capable variant of time.localtime
SourceForge.net
noreply at sourceforge.net
Fri Aug 10 01:16:26 CEST 2007
Patches item #1667546, was opened at 2007-02-24 00:25
Message generated for change (Comment added) made by pboddie
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1667546&group_id=5470
Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: Modules
Group: Python 2.6
Status: Open
Resolution: None
Priority: 5
Private: No
Submitted By: Paul Boddie (pboddie)
Assigned to: Georg Brandl (gbrandl)
Summary: Time zone-capable variant of time.localtime
Initial Comment:
Patch related to #1493676: "time.strftime() %z error"
This provides a localtime_tz function whose return value is the usual localtime time tuple with an additional field reflecting the underlying tm_gmtoff data. Various internal function signatures are modified to support the flow of time zone information, with the gettmarg most noticably changed (probably quite inelegantly - I don't do Python core development).
This patch is against the Python 2.4.4 release sources.
----------------------------------------------------------------------
>Comment By: Paul Boddie (pboddie)
Date: 2007-08-10 01:16
Message:
Logged In: YES
user_id=226443
Originator: YES
I've fixed some of the tests and consolidated some common code. I haven't
looked at METH_O yet because I'm not familiar with this stuff, and I can't
find a usable function to get the length of structure sequences which
includes the non-visible elements. I also don't recall why the ifdef was
changed, but I can look into that. As for the regexes, there's probably no
limit on how many hours an offset can be, so it may not make much sense to
be too clever about it.
I'd like to look at the glibc code for mktime to verify my assumptions.
Right now I don't have that much confidence in the new functions in the
patch because I may have misunderstood how certain C library functions
work. However, I have tested the revised code in DST and non-DST for CEST
and CET respectively (using User Mode Linux).
File Added: time-3.diff
----------------------------------------------------------------------
Comment By: Neal Norwitz (nnorwitz)
Date: 2007-08-07 08:48
Message:
Logged In: YES
user_id=33168
Originator: NO
Thanks for the patch Paul. As you know or learned, code that deals with
time is tricky. So I'm trying to be overly cautious with these comments.
Georg, I think this patch is fine to go in, but would like to see the
comments below addressed. I reviewed time-2.diff.
Should the regex for 'z' be tightened up in Lib/_strptime.py?
(\d\d[0-5]\d) It currently allows nonsense like: +5350. The correct
regex would be: ([01]\d|2[0-3])\d\d. I'm not sure it's worth it to go to
that much effort. Maybe [0-2]\d[0-5]\d? Probably best to be consistent
with the other regexes. What do the others do that parse hour?
No lines should be over 80 columns. I noticed a bunch of problems in the
test. Also it would be very helpful to breakup the assertions. This code
is likely to break (time calculations always do). The more info we have
when it breaks, the easier it will be do debug. So code like this:
self.assert_(lt.tm_zone is None and not hasattr(time, "tzname") or
lt.tm_zone == time.tzname[lt.tm_isdst])
Is probably better written something like this:
if lt.tm_zone is None:
self.assert_(not hasattr(time, "tzname"))
else:
self.assertEqual(lt.tm_zone, time.tzname[lt.tm_isdst])
That will provide a better message if the zones aren't equal and will also
be clear if the hasattr() test failed.
It will also fix the lines over 80 colums. :-)
Use assertEqual rather than assert_(x == y). For cases where you have to
use assert_(), it would be good to provide an error message about the
value(s).
Is the code in _tmtotuple_tz_helper and adjusttime() the same? If not,
they look very close, can you use a common utility function?
Use METH_O instead of METH_VARARGS for the new functions. That way you
don't need to call PyArgs_ParseTuple().
You are assuming that there are always at least 10 items in the structseq.
Is that valid? For example, what happens if you pickle a struct in python
2.5 and unpickle it in 2.6. Will it have the original length of 9 or the
new length of 10-11?
The code around PyType_IsSubtype() looks duplicated too. How about a
helper function for that?
Why is this code commented out at the end? /* && !defined(__GLIBC__) &&
!defined(__CYGWIN__) */
----------------------------------------------------------------------
Comment By: Paul Boddie (pboddie)
Date: 2007-08-04 02:18
Message:
Logged In: YES
user_id=226443
Originator: YES
I've updated the patch to work around redefinition of the tm_isdst field
by mktime, which appeared to defeat the measures put in place to support
mktimetz.
File Added: time-2.diff
----------------------------------------------------------------------
Comment By: Georg Brandl (gbrandl)
Date: 2007-07-12 11:51
Message:
Logged In: YES
user_id=849994
Originator: NO
I still have a failing test with the latest patch:
======================================================================
FAIL: test_mktimetz (test.test_time.TimeTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/gbr/devel/python/Lib/test/test_time.py", line 272, in
test_mktimetz
self.assert_(time.mktimetz(lt) == time.mktimetz(gt))
AssertionError
======================================================================
FAIL: test_timegm (test.test_time.TimeTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/gbr/devel/python/Lib/test/test_time.py", line 253, in
test_timegm
self.assert_(0 <= dt < 1)
AssertionError
----------------------------------------------------------------------
Ran 17 tests in 1.249s
FAILED (failures=2)
test test_time failed -- errors occurred; run in verbose mode for details
1 test failed:
test_time
----------------------------------------------------------------------
Comment By: Paul Boddie (pboddie)
Date: 2007-03-23 01:21
Message:
Logged In: YES
user_id=226443
Originator: YES
I've enhanced your version of the patch to work with the following
defines:
#define HAVE_TZNAME 1
#undef HAVE_TM_ZONE
#undef HAVE_STRUCT_TM_TM_ZONE
#undef HAVE_ALTZONE
It now uses struct_time in such an environment to provide the extra offset
information used in the unixtime function rather than accessing tm_gmtoff.
I suppose one could do that for all cases, in fact, since this is done
independently of any mktime invocation.
The above defines probably represent the next most "sane" of environments
after those which have tm_gmtoff. If one starts to remove other things,
other more established tests seem to fail, too.
File Added: time-1-improved.diff
----------------------------------------------------------------------
Comment By: Georg Brandl (gbrandl)
Date: 2007-03-21 11:25
Message:
Logged In: YES
user_id=849994
Originator: NO
I'm attaching a patch with some corrections of mine. It looks very good
now.
However, your tests fail if HAVE_STRUCT_TM_TM_ZONE is not defined. Can
that be repaired? If not, the tests must be skipped in this case.
File Added: time-1.diff
----------------------------------------------------------------------
Comment By: Paul Boddie (pboddie)
Date: 2007-03-20 01:20
Message:
Logged In: YES
user_id=226443
Originator: YES
File Added: tm_gmtoff_zone_timegm_mktimetz_26.diff
----------------------------------------------------------------------
Comment By: Paul Boddie (pboddie)
Date: 2007-03-20 01:19
Message:
Logged In: YES
user_id=226443
Originator: YES
Yet another round. Fixed timegm, hopefully. Added mktimetz which uses time
zone information to convert local and UMT times to UNIX times. Added tests
and updated the docs.
The usual caveats apply: I'm new to all this, so some things may need
sanity checking.
File Added: tm_gmtoff_zone_timegm_mktimetz.diff
----------------------------------------------------------------------
Comment By: Paul Boddie (pboddie)
Date: 2007-03-14 23:43
Message:
Logged In: YES
user_id=226443
Originator: YES
Looking at this a bit more, it seems like timegm (which is a pretty
desirable function to have) really is as simple as the calendar.timegm
function, as far as I can tell: it just throws time zone information away.
So the timegm implementation in the patches so far is actually wrong (and
broken in the way it attempts to use tm_gmtoff, anyway).
However, it might be nice to have a function which actually interprets
times properly in order to produce a UNIX time. In other words, something
which returns zero for both time.localtime(0) and time.gmtime(0), along
with other times which happen to refer to the epoch but in other time
zones.
I'll upload a fixed patch in the next day or so, hopefully. Sorry for the
noise!
----------------------------------------------------------------------
Comment By: Georg Brandl (gbrandl)
Date: 2007-03-12 15:13
Message:
Logged In: YES
user_id=849994
Originator: NO
The patch looks good so far -- but I'd be comfortable with a few more
tests, for example
for strptime and strftime.
Also, the documentation must be updated for every user-visible change (the
addition
of timegm(), the additional timetuple fields, the now-working (?) %z and
%Z in str[fp]time
and behavior changes).
----------------------------------------------------------------------
Comment By: Paul Boddie (pboddie)
Date: 2007-03-11 22:02
Message:
Logged In: YES
user_id=226443
Originator: YES
SourceForge "replay" added new attachment - now deleted.
----------------------------------------------------------------------
Comment By: Paul Boddie (pboddie)
Date: 2007-03-11 21:15
Message:
Logged In: YES
user_id=226443
Originator: YES
File Added: tm_gmtoff_zone_timegm_26.diff
----------------------------------------------------------------------
Comment By: Paul Boddie (pboddie)
Date: 2007-03-11 18:00
Message:
Logged In: YES
user_id=226443
Originator: YES
File Added: tm_gmtoff_zone_timegm_26.diff
----------------------------------------------------------------------
Comment By: Paul Boddie (pboddie)
Date: 2007-03-11 18:00
Message:
Logged In: YES
user_id=226443
Originator: YES
New patches against 2.4.4 and the trunk, providing tm_gmtoff, tm_zone,
strptime enhancements, a timegm function, plus tests. Where a platform
supports the timezone and altzone module attributes but not tm_gmtoff and
tm_zone struct tm fields, the code attempts to populate the struct_time
fields appropriately, setting None values only if no timezone information
exists whatsoever. Limitations include the inability to parse/obtain
timezone information beyond that provided by system calls, which is an
existing limitation that affects the strptime implementation amongst other
things.
Again, testing for "deficient" platforms with limited struct tm
definitions has been limited.
File Added: tm_gmtoff_zone_timegm.diff
----------------------------------------------------------------------
Comment By: Paul Boddie (pboddie)
Date: 2007-03-10 23:59
Message:
Logged In: YES
user_id=226443
Originator: YES
After some thought, perhaps the population of timezone fields is more
difficult than described below. Will need to look at other parts of the
module code to see what the complications are. What a headache!
----------------------------------------------------------------------
Comment By: Paul Boddie (pboddie)
Date: 2007-03-09 00:57
Message:
Logged In: YES
user_id=226443
Originator: YES
File Added: tm_gmtoff_zone_26.diff
----------------------------------------------------------------------
Comment By: Paul Boddie (pboddie)
Date: 2007-03-09 00:57
Message:
Logged In: YES
user_id=226443
Originator: YES
Attached are two patches (for 2.4.4 and against the trunk). Apart from the
addition of tm_zone, there's one big change: if no timezone fields/members
exist on struct tm, the code attempts to read that data from the module's
timezone and tzname attributes in order to populate tm_gmtoff and tm_zone;
if that fails then both tm_gmtoff and tm_zone are set to None.
The logic for all this is tested in test_time.py, but it really needs
checking for suitability and testing on something like HP-UX. Details for
the logic here:
http://devrsrc1.external.hp.com/STKT/impacts/i117.html
File Added: tm_gmtoff_zone.diff
----------------------------------------------------------------------
Comment By: Guido van Rossum (gvanrossum)
Date: 2007-03-08 01:52
Message:
Logged In: YES
user_id=6380
Originator: NO
No immediate time for review, but this sounds encouraging. Would you mind
adding tm_zone (a string) as well? And how about working relative to the
2.6 trunk (since that's the earliest version where this new feature can be
introduced)?
----------------------------------------------------------------------
Comment By: Paul Boddie (pboddie)
Date: 2007-03-08 01:28
Message:
Logged In: YES
user_id=226443
Originator: YES
One learns new things about time and stat tuples every day! Made a much
cleaner patch which provides an extra named attribute (tm_gmtoff) whilst
preserving the 9-tuple layout. Where no time zone support exists, tm_gmtoff
is None; otherwise it's the GMT/UTC offset.
I had weird test issues with range(7) giving "TypeError: an integer is
required" in the code employed (deep down) in test_strptime at some point
during development, probably due to memory issues, so it might be worth
checking that I've dealt properly with such things.
File Added: tm_gmtoff.diff
----------------------------------------------------------------------
Comment By: Guido van Rossum (gvanrossum)
Date: 2007-03-02 16:52
Message:
Logged In: YES
user_id=6380
Originator: NO
Without even looking at the patch, IMO it would be much better to add
tm_gmtoff and tm_zone (and any other fields) to the record returned by
localtime(), but in such a way that when accessed as a tuple it still has
only 9 fields. There is infrastructure for doing so somewhere for the stat
structure that I'm sure could be borrowed or generalized (if it isn't
already general). That's much better than adding a new function.
----------------------------------------------------------------------
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1667546&group_id=5470
More information about the Patches
mailing list