classification
Title: Time zone-capable variant of time.localtime
Type: enhancement Stage: resolved
Components: Extension Modules Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder: Add aware local time support to datetime module
View: 9527
Assigned To: belopolsky Nosy List: Alexander.Belopolsky, belopolsky, georg.brandl, gvanrossum, nnorwitz, pboddie, pitrou, python-dev, techtonik
Priority: normal Keywords: patch

Created on 2007-02-23 23:25 by pboddie, last changed 2012-06-14 02:18 by belopolsky. This issue is now closed.

Files
File name Uploaded Description Edit
timemodule.c.diff pboddie, 2007-02-23 23:25 Patch against timemodule.c providing localtime_tz and related enhancements
tm_gmtoff.diff pboddie, 2007-03-08 00:28 Revised patch against timemodule.c and test_time.py
tm_gmtoff_zone.diff pboddie, 2007-03-08 23:57 Revised patch against timemodule.c and test_time.py providing tm_zone (Python 2.4.4)
tm_gmtoff_zone_26.diff pboddie, 2007-03-08 23:57 Revised patch against timemodule.c and test_time.py providing tm_zone (trunk)
tm_gmtoff_zone_timegm.diff pboddie, 2007-03-11 17:00 Revised patch against timemodule.c, test_time.py, _strptime.py (Python 2.4.4)
tm_gmtoff_zone_timegm_26.diff pboddie, 2007-03-11 17:00 Revised patch against timemodule.c, test_time.py, _strptime.py (trunk)
tm_gmtoff_zone_timegm_mktimetz.diff pboddie, 2007-03-20 00:19 Revised patch against timemodule.c, test_time.py, _strptime.py, libtime.tex (Python 2.4.4)
tm_gmtoff_zone_timegm_mktimetz_26.diff pboddie, 2007-03-20 00:20 Revised patch against timemodule.c, test_time.py, _strptime.py, libtime.tex (trunk)
time-1.diff georg.brandl, 2007-03-21 10:25
time-1-improved.diff pboddie, 2007-03-23 00:21 time-1.diff improved to handle absent tm_gmtoff fields more reasonably
time-2.diff pboddie, 2007-08-04 00:18 Revised patch fixing tm_isdst issues
time-3.diff pboddie, 2007-08-09 23:16 Revised patch fixing tests, common code.
time-4.diff pboddie, 2007-08-10 23:47 Revised patch, adding sequence length tests, reverting ifdef change
time-4-py3k.diff belopolsky, 2010-06-06 20:37 Patch for py3k branch
issue1667546.diff belopolsky, 2012-06-13 21:51 review
Messages (37)
msg51917 - (view) Author: Paul Boddie (pboddie) Date: 2007-02-23 23:25
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.
msg51918 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-03-02 15:52
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.
msg51919 - (view) Author: Paul Boddie (pboddie) Date: 2007-03-08 00:28
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
msg51920 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2007-03-08 00:52
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)?
msg51921 - (view) Author: Paul Boddie (pboddie) Date: 2007-03-08 23:57
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
msg51922 - (view) Author: Paul Boddie (pboddie) Date: 2007-03-08 23:57
File Added: tm_gmtoff_zone_26.diff
msg51923 - (view) Author: Paul Boddie (pboddie) Date: 2007-03-10 22:59
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!
msg51924 - (view) Author: Paul Boddie (pboddie) Date: 2007-03-11 17:00
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
msg51925 - (view) Author: Paul Boddie (pboddie) Date: 2007-03-11 17:00
File Added: tm_gmtoff_zone_timegm_26.diff
msg51926 - (view) Author: Paul Boddie (pboddie) Date: 2007-03-11 20:15
File Added: tm_gmtoff_zone_timegm_26.diff
msg51927 - (view) Author: Paul Boddie (pboddie) Date: 2007-03-11 21:02
SourceForge "replay" added new attachment - now deleted.
msg51928 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2007-03-12 14:13
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).
msg51929 - (view) Author: Paul Boddie (pboddie) Date: 2007-03-14 22:43
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!
msg51930 - (view) Author: Paul Boddie (pboddie) Date: 2007-03-20 00:19
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
msg51931 - (view) Author: Paul Boddie (pboddie) Date: 2007-03-20 00:20
File Added: tm_gmtoff_zone_timegm_mktimetz_26.diff
msg51932 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2007-03-21 10:25
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
msg51933 - (view) Author: Paul Boddie (pboddie) Date: 2007-03-23 00:21
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
msg51934 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2007-07-12 09:51
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
msg51935 - (view) Author: Paul Boddie (pboddie) Date: 2007-08-04 00:18
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
msg51936 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2007-08-07 06:48
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__) */
msg51937 - (view) Author: Paul Boddie (pboddie) Date: 2007-08-09 23:16
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
msg51938 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2007-08-10 03:33
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?
msg51939 - (view) Author: Paul Boddie (pboddie) Date: 2007-08-10 23:47
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
msg82263 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-02-16 18:31
What is the status of this?
msg82398 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2009-02-18 00:32
To be honest, I forgot about it :(
msg107156 - (view) Author: anatoly techtonik (techtonik) Date: 2010-06-05 17:48
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.
msg107159 - (view) Author: Paul Boddie (pboddie) Date: 2010-06-05 18:52
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.
msg107226 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-06-06 20:37
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?
msg107231 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2010-06-06 21:22
(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.
msg125957 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2011-01-10 23:52
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.
msg125990 - (view) Author: anatoly techtonik (techtonik) Date: 2011-01-11 10:06
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.
msg126006 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2011-01-11 14:18
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?
msg162655 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2012-06-12 00:53
Reopening.  given the uncertainty with #9527, this issue may result in getting the TZ-aware local time support in stdlib sooner.
msg162733 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2012-06-13 21:51
I've simplified Paul's patch by removing timegm and mktimetz functions.  Also, platforms that don't support tm_zone are unaffected.
msg162740 - (view) Author: Paul Boddie (pboddie) Date: 2012-06-13 23:08
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
msg162744 - (view) Author: Alexander Belopolsky (Alexander.Belopolsky) Date: 2012-06-14 00:19
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.
msg162747 - (view) Author: Roundup Robot (python-dev) Date: 2012-06-14 02:16
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
History
Date User Action Args
2015-10-02 21:30:21belopolskylinkissue762963 superseder
2012-06-14 02:18:24belopolskysetstatus: open -> closed
resolution: fixed
stage: commit review -> resolved
2012-06-14 02:16:09python-devsetnosy: + python-dev
messages: + msg162747
2012-06-14 00:19:13Alexander.Belopolskysetnosy: + Alexander.Belopolsky
messages: + msg162744
2012-06-13 23:08:47pboddiesetmessages: + msg162740
2012-06-13 21:51:22belopolskysetfiles: + issue1667546.diff

messages: + msg162733
stage: patch review -> commit review
2012-06-12 00:53:48belopolskysetstatus: closed -> open
versions: + Python 3.3, - Python 3.2
messages: + msg162655

resolution: duplicate -> (no value)
stage: resolved -> patch review
2011-01-29 23:16:38belopolskysetstatus: open -> closed
nosy: gvanrossum, nnorwitz, georg.brandl, pboddie, belopolsky, pitrou, techtonik
2011-01-29 23:08:24belopolskylinkissue9527 dependencies
2011-01-11 14:18:00belopolskysetnosy: gvanrossum, nnorwitz, georg.brandl, pboddie, belopolsky, pitrou, techtonik
messages: + msg126006
2011-01-11 10:06:27techtoniksetstatus: pending -> open
nosy: gvanrossum, nnorwitz, georg.brandl, pboddie, belopolsky, pitrou, techtonik
messages: + msg125990
2011-01-10 23:52:22belopolskysetstatus: open -> pending
nosy: gvanrossum, nnorwitz, georg.brandl, pboddie, belopolsky, pitrou, techtonik
messages: + msg125957

superseder: Add aware local time support to datetime module
resolution: duplicate
stage: patch review -> resolved
2010-06-06 21:22:53georg.brandlsetassignee: georg.brandl -> belopolsky
messages: + msg107231
2010-06-06 20:37:34belopolskysetfiles: + time-4-py3k.diff

messages: + msg107226
2010-06-05 18:52:08pboddiesetmessages: + msg107159
2010-06-05 17:48:19techtoniksetnosy: + techtonik
messages: + msg107156
2010-05-26 01:05:41belopolskysetnosy: + belopolsky
2010-05-26 01:04:34belopolskysetstage: patch review
type: enhancement
versions: + Python 3.2, - Python 3.1, Python 2.7
2009-03-30 19:48:27ajaksu2linkissue1647654 dependencies
2009-03-30 03:09:05ajaksu2linkissue1560794 superseder
2009-02-18 00:32:24georg.brandlsetmessages: + msg82398
2009-02-16 18:31:52pitrousetnosy: + pitrou
messages: + msg82263
versions: + Python 3.1, Python 2.7, - Python 2.6
2008-01-06 14:21:48georg.brandllinkissue1245224 superseder
2007-02-23 23:25:33pboddiecreate