This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Author nnorwitz
Date 2007-08-07.06:48:06
SpamBayes Score
Marked as misclassified
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/  (\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"))
  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__) */
Date User Action Args
2007-08-23 15:56:41adminlinkissue1667546 messages
2007-08-23 15:56:41admincreate