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

#23657: Don't do isinstance checks in zipapp

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 2 months ago by brett
Modified:
4 years, 2 months ago
Reviewers:
p.f.moore
CC:
brett.cannon, pmoore, devnull_psf.upfronthosting.co.za
Visibility:
Public.

Patch Set 1 #

Patch Set 2 #

Total comments: 1

Patch Set 3 #

Total comments: 15

Patch Set 4 #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Doc/library/zipapp.rst View 1 2 3 2 chunks +9 lines, -8 lines 0 comments Download
Lib/test/test_zipapp.py View 1 2 3 4 chunks +99 lines, -0 lines 0 comments Download
Lib/zipapp.py View 1 2 3 8 chunks +24 lines, -9 lines 1 comment Download

Messages

Total messages: 5
brett.cannon
LGTM other than probably wanting one more test case. http://bugs.python.org/review/23657/diff/14209/Lib/test/test_zipapp.py File Lib/test/test_zipapp.py (right): http://bugs.python.org/review/23657/diff/14209/Lib/test/test_zipapp.py#newcode32 Lib/test/test_zipapp.py:32: ...
4 years, 2 months ago #1
brett.cannon
All of the "add a period" messages are because we promote complete sentences in comments. ...
4 years, 2 months ago #2
pmoore
http://bugs.python.org/review/23657/diff/14286/Lib/test/test_zipapp.py File Lib/test/test_zipapp.py (right): http://bugs.python.org/review/23657/diff/14286/Lib/test/test_zipapp.py#newcode294 Lib/test/test_zipapp.py:294: sys.argv = ['zipapp', str(source)] Yes, I thought about that. ...
4 years, 2 months ago #3
brett.cannon
One very minor bug to fix. Once it's fixed I would do one quick test ...
4 years, 2 months ago #4
pmoore
4 years, 2 months ago #5
> Lib/zipapp.py:194: main()
> You forgot to pass in sys.argv. =)

Actually that was deliberate :-) main() defaults args to None, which gets passed
direct to argparse, which in turn takes None to mean "use sys.argv".

It seems to work OK for me here on the command line as it stands. But if you
think explicitly passing sys.argv[1:] makes it easier to understand what's going
on, then I'm OK with doing that.
Sign in to reply to this message.

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