Issue 2759042: Review for issue #10233

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
d :	mark current comment as done

	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 or <Ctrl> + Enter :	save comment
<Esc> :	cancel edit

Issue 2759042: Review for issue #10233

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

Created:
13 years, 5 months ago by brett.cannon

Modified:
13 years, 5 months ago

Reviewers:
Antoine Pitrou

CC:
pitrou_free.fr

Base URL:
http://svn.python.org/projects/python/trunk/

Visibility:
Public.

Patch Set 1 #

Total comments: 6

Downloaded from: http://bugs.python.org/file19414/tarfileclose.patch

Created: 13 years, 5 months ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+52 lines, -35 lines)			Patch
		Lib/tarfile.py	View		1 chunk	+7 lines, -9 lines	2 comments	Download
		Lib/test/test_tarfile.py	View		9 chunks	+45 lines, -26 lines	4 comments	Download

Messages

Total messages: 3

Expand All Messages | Collapse All Messages

brett.cannon

http://codereview.appspot.com/2759042/diff/1/Lib/tarfile.py File Lib/tarfile.py (right): http://codereview.appspot.com/2759042/diff/1/Lib/tarfile.py#newcode1811 Lib/tarfile.py:1811: gzip.GzipFile(name, mode, compresslevel, fileobj), Make this 'finally' and rop ...

13 years, 5 months ago (2010-10-29 23:38:38 UTC) #2

Antoine Pitrou

13 years, 5 months ago (2010-10-29 23:46:02 UTC) #3

http://codereview.appspot.com/2759042/diff/1/Lib/tarfile.py
File Lib/tarfile.py (right):

http://codereview.appspot.com/2759042/diff/1/Lib/tarfile.py#newcode1811
Lib/tarfile.py:1811: gzip.GzipFile(name, mode, compresslevel, fileobj),
On 2010/10/29 23:38:39, brett.cannon wrote:
> Make this 'finally' and rop the 'raise' below

It would be wrong. The file must only be closed on error; on success, it is
stored on the instance and used later on (hence the "_extfileobj" attribute as
well).

http://codereview.appspot.com/2759042/diff/1/Lib/test/test_tarfile.py
File Lib/test/test_tarfile.py (right):

http://codereview.appspot.com/2759042/diff/1/Lib/test/test_tarfile.py#newcode55
Lib/test/test_tarfile.py:55: data = fobj.read()
On 2010/10/29 23:38:39, brett.cannon wrote:
> Any reason why you are using a try/finally instead of a context manager? Same
> goes for all the other places in this file where you don't use a context
> manager.

Because that file-like object doesn't support the context manager (which could
be the subject of a separate issue, I suppose).

http://codereview.appspot.com/2759042/diff/1/Lib/test/test_tarfile.py#newcode152
Lib/test/test_tarfile.py:152: 
On 2010/10/29 23:38:39, brett.cannon wrote:
> If the assertEqual fails these files won't be closed. Better to put them in a
> nexted context manager.

Ok, will do.

Expand All Messages | Collapse All Messages