Don't do isinstance checks in zipapp

[brettcannon]

BPO	23657
Nosy	@brettcannon, @pfmoore
Files	duck_typed_zipapp.patch duck_typed_zipapp.patch duck_typed_zipapp.v3.patch duck_typed_zipapp.v4.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/pfmoore'
closed_at = <Date 2015-03-22.16:16:03.645>
created_at = <Date 2015-03-13.14:50:10.828>
labels = ['type-feature', 'library']
title = "Don't do isinstance checks in zipapp"
updated_at = <Date 2015-03-22.16:16:03.644>
user = 'https://github.com/brettcannon'

bugs.python.org fields:

activity = <Date 2015-03-22.16:16:03.644>
actor = 'paul.moore'
assignee = 'paul.moore'
closed = True
closed_date = <Date 2015-03-22.16:16:03.645>
closer = 'paul.moore'
components = ['Library (Lib)']
creation = <Date 2015-03-13.14:50:10.828>
creator = 'brett.cannon'
dependencies = []
files = ['38472', '38513', '38606', '38623']
hgrepos = []
issue_num = 23657
keywords = ['patch']
message_count = 10.0
messages = ['238030', '238031', '238033', '238038', '238084', '238085', '238243', '238685', '238818', '238913']
nosy_count = 3.0
nosy_names = ['brett.cannon', 'paul.moore', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue23657'
versions = ['Python 3.5']

[brettcannon]

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.

[pfmoore]

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?

[brettcannon]

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.

[pfmoore]

Looks good. Would it be worth adding tests for providing pathlib.Path objects, or is that overkill?

[brettcannon]

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.

[pfmoore]

Cool, I'll look at sorting it out.

[pfmoore]

Updated version of the patch with tests, plus doc update noting that path objects are explicitly supported.

[pfmoore]

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.

[pfmoore]

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.

[python-dev]

New changeset 0b2993742650 by Paul Moore in branch 'default':
bpo-23657 Don't explicitly do an isinstance check for str in zipapp
https://hg.python.org/cpython/rev/0b2993742650