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

Fixes the regexp to make it match the filenames produced by setuptools_scm #269

Closed
wants to merge 2 commits into from

Conversation

KOLANICH
Copy link

No description provided.

@benoit-pierre
Copy link
Member

Why not use the same one as pip?

@codecov
Copy link

codecov bot commented Oct 17, 2018

Codecov Report

Merging #269 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #269   +/-   ##
=======================================
  Coverage   59.49%   59.49%           
=======================================
  Files          12       12           
  Lines         827      827           
=======================================
  Hits          492      492           
  Misses        335      335
Impacted Files Coverage Δ
wheel/wheelfile.py 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8006c9...6a79cbb. Read the comment docs.

@KOLANICH
Copy link
Author

KOLANICH commented Oct 17, 2018

Why not use the same one as pip?

It is too broad so it was replaced here in c84867f breaking compatibility. IMHO it makes no sense to check wheel filename at all.

@agronholm
Copy link
Contributor

What standard includes a commit hash in the wheel file name?

@agronholm
Copy link
Contributor

FWIW, fixing was never blocked me making code changes, but getting a definitive answer from @dholth regarding the interpretation of PEP 427.

@KOLANICH
Copy link
Author

What standard includes a commit hash in the wheel file name?

Not standard, but usage of pypa/setuptools_scm causes this kind of filenames to be produced. It is strange to see that pypa/wheel broken compatibility to pypa/setuptools_scm. This made some packages non-installable from git. I insist on fixing compatibility as soon as possible, since this breaks packages, especially my packages, and only then standardizing it. Or non-standardizing and taking measures to prevent this part from appearing in filename. I like the first variant better.

@agronholm
Copy link
Contributor

We have an accepted standard saying that any character not matching [\w\d.] should be replaced by an underscore. I will fix this by making the regex even more permissible until we get a more definitive answer from the person who wrote the PEP.

@agronholm
Copy link
Contributor

I made my own changes to the regex already so I'm closing this one.

@agronholm agronholm closed this Oct 18, 2018
@dholth
Copy link
Member

dholth commented Oct 18, 2018 via email

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.

4 participants