classification
Title: Don't do isinstance checks in zipapp
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: paul.moore Nosy List: brett.cannon, paul.moore, python-dev
Priority: normal Keywords: patch

Created on 2015-03-13 14:50 by brett.cannon, last changed 2015-03-22 16:16 by paul.moore. This issue is now closed.

Files
File name Uploaded Description Edit
duck_typed_zipapp.patch brett.cannon, 2015-03-13 15:12 review
duck_typed_zipapp.patch paul.moore, 2015-03-16 21:46 review
duck_typed_zipapp.v3.patch paul.moore, 2015-03-20 14:41 review
duck_typed_zipapp.v4.patch paul.moore, 2015-03-21 17:13 review
Messages (10)
msg238030 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2015-03-13 14:50
As it stand, zipapp's code checks for str and then otherwise assumes an object is a file-like object. It might work out better to do some duck typing and simply check if an object has 'read' and 'readline' attributes then it's a file-like object and otherwise that it's a path.

Doing this would then potentially make it easier to use pathlib.Path through the module rather than the os module.
msg238031 - (view) Author: Paul Moore (paul.moore) * (Python committer) Date: 2015-03-13 15:04
That sounds reasonable. I'll have a look at this. The code was originally based on a similar pattern in the zipfile module, so maybe zipfile should be changed in the same way?
msg238033 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2015-03-13 15:12
Here is a proposed patch that does what I was thinking. What do you think?

As for updating zipfile, it's possible but slightly risky because of backwards-compatibility. Since this is all-new code there is no worry about breaking someone's pre-existing code because they were relying on str being treated in a special way instead of the file-like object case.
msg238038 - (view) Author: Paul Moore (paul.moore) * (Python committer) Date: 2015-03-13 15:43
Looks good. Would it be worth adding tests for providing pathlib.Path objects, or is that overkill?
msg238084 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2015-03-14 13:43
Depends on whether you want to support pathlib.Path objects explicitly. =) If so then yes, we should add tests.

Which reminds me, it might be time for you to request commit privileges so you can commit patches like this yourself.
msg238085 - (view) Author: Paul Moore (paul.moore) * (Python committer) Date: 2015-03-14 13:47
Cool, I'll look at sorting it out.
msg238243 - (view) Author: Paul Moore (paul.moore) * (Python committer) Date: 2015-03-16 21:46
Updated version of the patch with tests, plus doc update noting that path objects are explicitly supported.
msg238685 - (view) Author: Paul Moore (paul.moore) * (Python committer) Date: 2015-03-20 14:41
Updated patch. Added the requested test and a set of tests for the command line API. This also highlighted a bug in the command line --info option, which is also fixed.

Coverage now:

Name          Stmts   Miss  Cover   Missing
-------------------------------------------
test_zipapp     254     12    95%   252-257, 263-268
zipapp          102      2    98%   30, 192
-------------------------------------------
TOTAL           356     14    96%

Remaining misses are Unix-only tests (this was run on Windows) and the __main__ block.
msg238818 - (view) Author: Paul Moore (paul.moore) * (Python committer) Date: 2015-03-21 17:13
Updated patch with fixes for review comments. I did remove the tests for the exact error messages, as testing for a non-zero exit code was actually what I was trying to do, and I found a better way of doing that.
msg238913 - (view) Author: Roundup Robot (python-dev) Date: 2015-03-22 15:33
New changeset 0b2993742650 by Paul Moore in branch 'default':
#23657 Don't explicitly do an isinstance check for str in zipapp
https://hg.python.org/cpython/rev/0b2993742650
History
Date User Action Args
2015-03-22 16:16:03paul.mooresetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2015-03-22 15:33:18python-devsetnosy: + python-dev
messages: + msg238913
2015-03-21 17:13:36paul.mooresetfiles: + duck_typed_zipapp.v4.patch

messages: + msg238818
2015-03-20 14:41:55paul.mooresetfiles: + duck_typed_zipapp.v3.patch

messages: + msg238685
2015-03-16 21:46:59paul.mooresetfiles: + duck_typed_zipapp.patch

messages: + msg238243
2015-03-14 13:47:18paul.mooresetmessages: + msg238085
2015-03-14 13:43:03brett.cannonsetmessages: + msg238084
2015-03-13 15:43:03paul.mooresetmessages: + msg238038
2015-03-13 15:12:23brett.cannonsetfiles: + duck_typed_zipapp.patch
keywords: + patch
messages: + msg238033

stage: needs patch -> patch review
2015-03-13 15:04:53paul.mooresetmessages: + msg238031
2015-03-13 14:50:10brett.cannoncreate