Review for Python issue 5094 - Code Review (original) (raw)

Issue 22042: Review for Python issue 5094 (Closed)

Can't Edit Can't Publish+Mail Start Review Created: 16 years, 2 months ago by brett.cannon Modified: 14 years, 11 months ago Reviewers: ajaksu CC: RafeChops Base URL: http://svn.python.org/view/\*checkout\*/python/trunk/ Visibility: Public.	Patch Set 1# Total comments: 8 Downloaded from: http://bugs.python.org/file13029/datetime-utc-doc.patch Created: 16 years, 2 months ago Download[raw] [tar.bz2] Unified diffs Side-by-side diffs Delta from patch set Stats (+167 lines, -33 lines) Patch Doc/includes/tzinfo-examples.py View 1 chunk +0 lines, -16 lines 1 comment Download Doc/library/datetime.rst View 5 chunks +25 lines, -11 lines 1 comment Download Include/datetime.h View 2 chunks +6 lines, -0 lines 0 comments Download Lib/test/test_datetime.py View 5 chunks +21 lines, -1 line 1 comment Download Modules/datetimemodule.c View 9 chunks +115 lines, -5 lines 5 comments Download Messages Total messages: 3 Expand All Messages | Collapse All Messages brett.cannon 16 years, 2 months ago (2009-02-27 03:45:09 UTC)#1 Sign in to reply to this message. brett.cannon Looks good overall with minor fixes needed, although two things are missing. One, there are ... 16 years, 2 months ago (2009-02-27 04:12:29 UTC)#2 Looks good overall with minor fixes needed, although two things are missing. One, there are no tests for the new argument to datetime.utcnow(). Two, PyDateTime_CAPI from datetime.h should probably grow a UTC constructor field (along with the proper changes in datetimemodule.c). http://codereview.appspot.com/22042/diff/1/5 File Lib/test/test_datetime.py (right): http://codereview.appspot.com/22042/diff/1/5#newcode135 Line 135: self.assertEquals(timedelta(0), UTC().utcoffset(None)) Why the test for the None argument? Docs don't seem to suggest this is an acceptable argument. Same question applies to the two tests below. http://codereview.appspot.com/22042/diff/1/6 File Modules/datetimemodule.c (right): http://codereview.appspot.com/22042/diff/1/6#newcode3060 Line 3060: utc_init(PyObject *self) This function is never used. http://codereview.appspot.com/22042/diff/1/6#newcode3085 Line 3085: {"tzname", (PyCFunction)utc_tzname, METH_O, Ditch the tab spacing. http://codereview.appspot.com/22042/diff/1/6#newcode3098 Line 3098: PyDoc_STR("UTC timezone class."); Mention it is an implementation of tzinfo. http://codereview.appspot.com/22042/diff/1/6#newcode3947 Line 3947: if (tz_aware == Py_True) Use PyObject_IsTrue() instead of checking for Py_True directly. http://codereview.appspot.com/22042/diff/1/6#newcode3948 Line 3948: utc = new_utc(); Check for errors on the return value. Sign in to reply to this message. ajaksu Two tiny suggestions/questions :) http://codereview.appspot.com/22042/diff/1/4 File Doc/includes/tzinfo-examples.py (left): http://codereview.appspot.com/22042/diff/1/4#oldcode6 Line 6: # A UTC class. ... 16 years, 2 months ago (2009-02-27 04:38:00 UTC)#3 Two tiny suggestions/questions :) http://codereview.appspot.com/22042/diff/1/4 File Doc/includes/tzinfo-examples.py (left): http://codereview.appspot.com/22042/diff/1/4#oldcode6 Line 6: # A UTC class. I think this bit of unused code did have some educational value... maybe it could be added to datetime.rst? http://codereview.appspot.com/22042/diff/1/3 File Doc/library/datetime.rst (left): http://codereview.appspot.com/22042/diff/1/3#oldcode36 Line 36: application. The rules for time adjustment across the world are more political Maybe a (footnote) mention of/ link to pytz could add value for users reading this doc? Sign in to reply to this message. Expand All Messages	Collapse All Messages

Issue 22042: Review for Python issue 5094 (Closed)
Created 16 years, 2 months ago by brett.cannon
Modified 14 years, 11 months ago
Reviewers: ajaksu
Base URL: http://svn.python.org/view/\*checkout\*/python/trunk/
Comments: 8