Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't do isinstance checks in zipapp #67845

Closed
brettcannon opened this issue Mar 13, 2015 · 10 comments
Closed

Don't do isinstance checks in zipapp #67845

brettcannon opened this issue Mar 13, 2015 · 10 comments
Assignees
@pfmoore
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@brettcannon
Copy link
Member

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
    Copy link
    Member Author

    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.

    @brettcannon brettcannon added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Mar 13, 2015
    @pfmoore
    Copy link
    Member

    pfmoore commented Mar 13, 2015

    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
    Copy link
    Member Author

    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
    Copy link
    Member

    pfmoore commented Mar 13, 2015

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

    @brettcannon
    Copy link
    Member Author

    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
    Copy link
    Member

    pfmoore commented Mar 14, 2015

    Cool, I'll look at sorting it out.

    @pfmoore
    Copy link
    Member

    pfmoore commented Mar 16, 2015

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

    @pfmoore
    Copy link
    Member

    pfmoore commented Mar 20, 2015

    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
    Copy link
    Member

    pfmoore commented Mar 21, 2015

    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
    Copy link
    Mannequin

    python-dev mannequin commented Mar 22, 2015

    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

    @pfmoore pfmoore closed this as completed Mar 22, 2015
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees

    @pfmoore pfmoore

    Labels
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    2 participants
    @brettcannon @pfmoore