[issue23657] Don't do isinstance checks in zipapp
Roundup Robot added the comment: 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 -- nosy: +python-dev ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23657 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23657] Don't do isinstance checks in zipapp
Changes by Paul Moore p.f.mo...@gmail.com: -- resolution: - fixed stage: patch review - resolved status: open - closed ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23657 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23657] Don't do isinstance checks in zipapp
Paul Moore added the comment: 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. -- Added file: http://bugs.python.org/file38623/duck_typed_zipapp.v4.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23657 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23657] Don't do isinstance checks in zipapp
Paul Moore added the comment: 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 1295% 252-257, 263-268 zipapp 102 298% 30, 192 --- TOTAL 356 1496% Remaining misses are Unix-only tests (this was run on Windows) and the __main__ block. -- Added file: http://bugs.python.org/file38606/duck_typed_zipapp.v3.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23657 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23657] Don't do isinstance checks in zipapp
Paul Moore added the comment: Updated version of the patch with tests, plus doc update noting that path objects are explicitly supported. -- Added file: http://bugs.python.org/file38513/duck_typed_zipapp.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23657 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23657] Don't do isinstance checks in zipapp
Brett Cannon added the comment: 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. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23657 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23657] Don't do isinstance checks in zipapp
Paul Moore added the comment: Cool, I'll look at sorting it out. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23657 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23657] Don't do isinstance checks in zipapp
New submission from Brett Cannon: 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. -- assignee: pmoore components: Library (Lib) messages: 238030 nosy: brett.cannon, pmoore priority: normal severity: normal stage: needs patch status: open title: Don't do isinstance checks in zipapp type: enhancement versions: Python 3.5 ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23657 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23657] Don't do isinstance checks in zipapp
Paul Moore added the comment: 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? -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23657 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23657] Don't do isinstance checks in zipapp
Paul Moore added the comment: Looks good. Would it be worth adding tests for providing pathlib.Path objects, or is that overkill? -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23657 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23657] Don't do isinstance checks in zipapp
Brett Cannon added the comment: 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. -- keywords: +patch stage: needs patch - patch review Added file: http://bugs.python.org/file38472/duck_typed_zipapp.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23657 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
