Issue 665194: datetime-RFC2822 roundtripping

Keyboard Shortcuts

	File
u :	up to issue
m :	publish + mail comments
M :	edit review message
j / k :	jump to file after / before current file
J / K :	jump to next file with a comment after / before current file
	Side-by-side diff
i :	toggle intra-line diffs
e :	expand all comments
c :	collapse all comments
s :	toggle showing all comments
n / p :	next / previous diff chunk or comment
N / P :	next / previous comment
<Up> / <Down> :	next / previous line
<Enter> :	respond to / edit current comment

	Issue
u :	up to list of issues
m :	publish + mail comments
j / k :	jump to patch after / before current patch
o / <Enter> :	open current patch in side-by-side view
i :	open current patch in unified diff view

	Issue List
j / k :	jump to issue after / before current issue
o / <Enter> :	open current issue
# :	close issue

	Comment/message editing
<Ctrl> + s :	save comment
<Esc> :	cancel edit

Issue 665194: datetime-RFC2822 roundtripping

Can't Edit
Can't Publish+Mail
Start Review

Created:
1 year, 7 months ago by walter

Modified:
10 months, 1 week ago

Reviewers:
belopolsky, merwok

CC:
tim.peters_gmail.com, lists_amk.ca, walter_livinglogic.de, brett.cannon, sasha, ajaksu_gmail.com, eric.araujo, r.david.murray, devnull_psf.upfronthosting.co.za

Visibility:
Public.

Patch Set 1 #

Patch Set 2 #

Total comments: 8

Patch Set 3 #

Created: 10 months, 2 weeks ago

Download raw patch set

Unified diffs	Side-by-side diffs	Delta from patch set	Stats			Patch
Doc/library/email.util.rst	View		2 chunks	+18 lines, -0 lines	0 comments	Download
Lib/email/utils.py	View	2	5 chunks	+35 lines, -7 lines	0 comments	Download
Lib/test/test_email/test_utils.py	View		1 chunk	+40 lines, -0 lines	0 comments	Download

Messages

Total messages: 2

Expand All Messages | Collapse All Messages

belopolsky_users.sourceforge.net

Posting partial review. I don't like the proposed API - will comment on the tracker. ...

1 year ago #1

Posting partial review.  I don't like the proposed API - will comment on the
tracker.

http://bugs.python.org/review/665194/diff/2542/6110
File Doc/library/email.util.rst (right):

http://bugs.python.org/review/665194/diff/2542/6110#newcode102
Doc/library/email.util.rst:102: is used.  *timeval* may also be a
:class:`~datetime.datetime` instance.  If
Please use naive/aware datetime terminology instead of referring to tzinfo
member.

http://bugs.python.org/review/665194/diff/2542/6110#newcode103
Doc/library/email.util.rst:103: the intsance does not have a
:class:`~datetime.tzinfo`, the behavior is as
Spelling: instance.

http://bugs.python.org/review/665194/diff/2542/6110#newcode104
Doc/library/email.util.rst:104: with a floating point time value.  If it does
have a
This is not clear.  Floating point value is unambiguous TZ independent "seconds
since epoch".  Naive datetime can be either UTC or local.

http://bugs.python.org/review/665194/diff/2542/6111
File Lib/email/utils.py (right):

http://bugs.python.org/review/665194/diff/2542/6111#newcode119
Lib/email/utils.py:119: gmtime() and localtime(), otherwise the current time is
used.  timeval
Please don't start sentence with a word that has to be in lowercase.  Use
"Argument timeval' or rephrase.

http://bugs.python.org/review/665194/diff/2542/6111#newcode122
Lib/email/utils.py:122: a tzinfo, then localtime and usegmt are ignored and the
output string
I am not sure it is a good idea to further complicate formatdate() function
which already has two constant bool arguments.  This is already a design red
flag suggesting that several functions may be a better choice.  I would just add
a separate function dedicated to formatting datetime.

http://bugs.python.org/review/665194/diff/2542/6111#newcode138
Lib/email/utils.py:138: elif hasattr(timeval, 'timetuple'):
I would do isinstance(timeval, datetime) instead.  Having timetuple method is
not enough for duck typing here.  Are there 3rd party date/time classes in the
wild that will work with this implementation?

eric.araujo

10 months, 1 week ago #2

http://bugs.python.org/review/665194/diff/2542/6111
File Lib/email/utils.py (right):

http://bugs.python.org/review/665194/diff/2542/6111#newcode119
Lib/email/utils.py:119: gmtime() and localtime(), otherwise the current time is
used.  timeval
> Please don't start sentence with a word that has to be in
> lowercase.  Use "Argument timeval' or rephrase.

I’ve also seen markup like “*timeval*” used in such contexts.

http://bugs.python.org/review/665194/diff/2542/6111#newcode154
Lib/email/utils.py:154: if time.daylight and now[-1]>0:
What about a couple spaces around the operator?

Expand All Messages | Collapse All Messages