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.

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.

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

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)?

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

File Added: tm_gmtoff_zone_26.diff

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!

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

File Added: tm_gmtoff_zone_timegm_26.diff

File Added: tm_gmtoff_zone_timegm_26.diff

SourceForge "replay" added new attachment - now deleted.

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).

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!

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

File Added: tm_gmtoff_zone_timegm_mktimetz_26.diff

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

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

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

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

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__) */

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

METH_O is easy.  Change METH_VARARGS to METH_O and the only difference is that in your function:

static void
my_meth_func(PyObject* self, PyObject* arg)

arg is now the single argument.  So you don't need to call PyArg_ParseTuple.  The code that originally looked like:


static void
my_meth_func(PyObject* self, PyObject* args) {

PyArg_ParseTuple(args, "O", &my_arg);
// use my_arg

becomes:

static void
my_meth_func(PyObject* self, PyObject* my_arg) {
// use my_arg

Hope that makes sense.

As for the length of visible vs non-visible attributes, can't you use the macros as the top of structseq.c file?

VISIBLE_SIZE, REAL_SIZE, UNNAMED_FIELDS and the _TP variants?

I've changed the new functions to use METH_O, and I've copied the macros in from structseq.c, although these should really be exposed somewhere common, I imagine. Consequently, tests around usage of the structure are now in place, and I've added a test of unpickling an old time object to the test suite.

The reason for the changed ifdef is something I don't recall, so I've reverted that change as it doesn't seem to change the behaviour of the code when tested.
File Added: time-4.diff

What is the status of this?

To be honest, I forgot about it :(