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 :(

Yet another timezone issue. =/ Seems like it is too complicated.

Should we try to organize a dedicated sprint during EuroPython?
Do we need some easy to read research on the problem?
Should PSF define bounty for that?

Should we try to approach this with some other entrypoint like high level user story? I can play the role of the dumb user. Please, add your replies to this Wave https://wave.google.com/wave/waveref/googlewave.com/w+FbjMbPbEA so that we can develop the story.

Speaking for myself, I'm not sure whether I'm really the person to push this further, at least, although others may see it as a worthy sprinting topic. In principle, adding the extra fields is the right thing to do, merely because it exposes things from "struct tm" which were missing and which influence the other functions depending on it.

The only things remaining are to make sure that existing code doesn't change its behaviour with these new fields, and that the new fields work together with the time functions as expected. Thus, testing is the most important thing now, I think.

For the bigger picture, which shouldn't be discussed here, Python's way of handling times and dates probably needs improvement - this has been discussed already (a reference for anyone not involved is Anatoly's initial message in one recent discussion: http://mail.python.org/pipermail/python-dev/2010-February/097710.html) - and I think usage of pytz is a step in the right direction, although it does not eliminate the need to learn about time zones (UTC, CET...), time "regimes" (Europe/Oslo, America/New_York...), "floating" times, and zone transitions (and ambiguous times).

Extending Python library support is potentially a sprinting topic, but not really a topic for discussion around this patch.

I have updated time-4.diff patch for py3k.  Code changes are minimal, mostly due to string to unicode conversion. I have not attempted to fix anything beyond passing supplied unittest, but I did noticed a few missing error return tests.  I was not able to make the final test in test_mktimetz pass because I did not completely understand its logic.

Georg,

I am trying to consolidate and bring to resolution a number to timezone related issues.  Would you like me to take this issue from you or do you want to continue to own it?

(Hi Alexander, sorry for not answering on IRC, I got a phonecall and was distracted for a while.)  Please take on this issue -- I've simply not had energy enough to fight it through.

I am going to close this as superseded by #9527.  In order to properly implement #9527 in datetime.py, some kind of tm_gmtoff support will need to be added to the time module, but I don't want this to become a feature that is exclusively available from the time module.

IIUC #9527 is about datetime and this request is about time.localtime, i.e. about making the API more intuitive for users. I don't think this issue should be closed.

On Tue, Jan 11, 2011 at 5:06 AM, anatoly techtonik
<report@bugs.python.org> wrote:
..
> IIUC #9527 is about datetime and this request is about time.localtime,

This is correct, but did you notice what I wrote in my last message?

"""
In order to properly implement #9527 in datetime.py, some kind of
tm_gmtoff support will need to be added to the time module, but I
don't want this to become a feature that is exclusively available from
the time module.
"""

I am not rejecting this request, I am trying to consolidate two
closely related issues.  Can you explain why you believe this
functionality should be provided exclusively through the time module
and not be available in datetime?

Reopening.  given the uncertainty with #9527, this issue may result in getting the TZ-aware local time support in stdlib sooner.

I've simplified Paul's patch by removing timegm and mktimetz functions.  Also, platforms that don't support tm_zone are unaffected.

On Wednesday 13 June 2012 23:51:25 Alexander Belopolsky wrote:
> Alexander Belopolsky <alexander.belopolsky@gmail.com> added the comment:
>
> I've simplified Paul's patch by removing timegm and mktimetz functions. 
> Also, platforms that don't support tm_zone are unaffected.

I think you may have forgotten to remove docstring references to those 
functions. Nice to see some progress on the issue, though, and it's probably 
good to solve one problem at a time in this way, too.

Paul

On Wed, Jun 13, 2012 at 7:08 PM, Paul Boddie <report@bugs.python.org> wrote:
> I think you may have forgotten to remove docstring references to those
> functions.

Good catch.  BTW, did you write the additional tests for strptime?
This is the only thing that I want to add before committing.

New changeset 3b5545ba6432 by Alexander Belopolsky in branch 'default':
Issue #1667546: On platforms supporting tm_zone and tm_gmtoff fields
http://hg.python.org/cpython/rev/3b5545ba6432