Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(194733)

#22217: Reprs for zipfile classes

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 3 months ago by storchaka+cpython
Modified:
5 years, 1 month ago
Reviewers:
berker.peksag
CC:
ezio.melotti, devnull_psf.upfronthosting.co.za, storchaka
Visibility:
Public.

Patch Set 1 #

Total comments: 1

Patch Set 2 #

Total comments: 9
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Lib/test/test_zipfile.py View 1 1 chunk +31 lines, -0 lines 0 comments Download
Lib/zipfile.py View 1 3 chunks +50 lines, -0 lines 9 comments Download

Messages

Total messages: 4
berkerpeksag
It would be good to add a simple test case for reprs (e.g. check '[closed]' ...
5 years, 3 months ago #1
berkerpeksag
Mostly nitpicks. The patch LGTM. http://bugs.python.org/review/22217/diff/12750/Lib/zipfile.py File Lib/zipfile.py (right): http://bugs.python.org/review/22217/diff/12750/Lib/zipfile.py#newcode367 Lib/zipfile.py:367: result.append(' filemode=%r' % (stat.filemode(hi),)) ...
5 years, 2 months ago #2
storchaka_gmail.com
http://bugs.python.org/review/22217/diff/12750/Lib/zipfile.py File Lib/zipfile.py (right): http://bugs.python.org/review/22217/diff/12750/Lib/zipfile.py#newcode367 Lib/zipfile.py:367: result.append(' filemode=%r' % (stat.filemode(hi),)) On 2014/10/01 01:40:47, berkerpeksag wrote: ...
5 years, 1 month ago #3
berkerpeksag
5 years, 1 month ago #4
On 2014/10/08 19:42:09, storchaka wrote:
> http://bugs.python.org/review/22217/diff/12750/Lib/zipfile.py
> File Lib/zipfile.py (right):
> 
> http://bugs.python.org/review/22217/diff/12750/Lib/zipfile.py#newcode367
> Lib/zipfile.py:367: result.append(' filemode=%r' % (stat.filemode(hi),))
> On 2014/10/01 01:40:47, berkerpeksag wrote:
> > Passing a tuple is not necessary here.
> > 
> >     ' filemode=%r' % stat.filemode(hi)
> > 
> > would be clearer.
> 
> I were too paranoid here. When right argument of the % operator unexpectedly
> becomes a tuple, this causes an error. But may be in these cases we can ignore
> this possibility.

Ah, I see, thanks. What about using str.format then?
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7+