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.

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

Added support for wheel files with local version (pep-0440) #38

Merged
merged 2 commits into from
Jul 10, 2018

Conversation

bsiggers
Copy link

@bsiggers bsiggers commented Jul 6, 2018

Fixed an issue where wheel files with local version identifiers (eg. f-1.0+local-py3-none-any.whl) were being rejected as having unsafe package names, since '+' was being rejected based on the filename regex in Package.create. '+' is a permitted character in wheel filenames as per PEP-0440 (https://www.python.org/dev/peps/pep-0440/#local-version-identifiers).

@@ -104,6 +104,14 @@ def test_package_info_all_info():
}


def test_package_info_wheel_with_local_version():
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of a new test, I think you can just add a case to test_guess_name_version_from_filename above

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asottile - I considered that, but that test function doesn't cover the change - guess_name_version_from_filename() is called from inside create(), but only after the regex gets checked.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh ok, let's go for a more directed test then, how about something like this?

    pkg = main.Package.create(filename='f-1.0+local-py3-none-any.whl')
    assert pkg.version == '1.0+local'

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asottile - done

@@ -150,7 +150,7 @@ def create(
upload_timestamp: Optional[int] = None,
uploaded_by: Optional[str] = None,
) -> 'Package':
if not re.match('[a-zA-Z0-9_\-\.]+$', filename) or '..' in filename:
if not re.match('[a-zA-Z0-9_\-\.\+]+$', filename) or '..' in filename:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while you're at it (not your fault, existing code -- sorry!) can you add a r prefix to this regex -- (it'll become a syntax error in python3.8 -- heh)

@asottile
Copy link
Collaborator

asottile commented Jul 7, 2018

forgot to say, thanks for this PR -- definitely was an oversight on our part and we're happy to get this feature working 🎉

@bsiggers
Copy link
Author

@asottile - no trouble, thanks to you guys for creating & maintaining this package, saved us a bunch of time.

@asottile asottile merged commit 4a8edf8 into chriskuehl:master Jul 10, 2018
@asottile
Copy link
Collaborator

This has been released as part of v1.3.1 -- thanks again for your contribution 🎉 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants