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

fix: correct parsing of wheel version with regex. #1932

Merged
merged 1 commit into from Jan 22, 2020

Conversation

@edwardgeorge
Copy link
Contributor

edwardgeorge commented Jan 22, 2020

The previous regexp was only taking the first integer of the version number,
this presented problems when the major version number reached double digits.

Poetry would determine that the version of the dependency is '1', rather than,
ie: '14'. This caused failures to solve versions.

Detailed description of issue in bug report that I filed here: #1931

This fixes the issue for me and allows poetry update to correctly solve versions when a wheel file is referenced with a version >=10.

Unrelated to the specifics of this bug, but I noticed that when parsing a url to a wheel it does not return a URLDependency like non-wheel based urls but a Dependency instance. Therefore dep.is_url() returns False and the dependency only contains the name and parsed version as the constraint, the url itself is discarded. I'm not sure if this is intended or not?

Pull Request Check List

This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. But please read our contribution guide at least once, it will save you unnecessary review cycles!

  • Added tests for changed code.
  • Updated documentation for changed code. not appropriate
@edwardgeorge edwardgeorge requested a review from sdispater Jan 22, 2020
@edwardgeorge edwardgeorge force-pushed the edwardgeorge:master branch from 2e24f26 to 88277ed Jan 22, 2020
Copy link
Member

finswimmer left a comment

Looks good to me! Thanks a lot for your contribution 👍

@@ -70,7 +70,7 @@ def dependency_from_pep_508(name):
link = Link(path_to_url(os.path.normpath(os.path.abspath(link.path))))
# wheel file
if link.is_wheel:
m = re.match(r"^(?P<namever>(?P<name>.+?)-(?P<ver>\d.*?))", link.filename)
m = re.match(r"^(?P<namever>(?P<name>.+?)-(?P<ver>\d[^-]*))", link.filename)

This comment has been minimized.

Copy link
@sdispater

sdispater Jan 22, 2020

Member

I think we can reuse the wheel_file_re from poetry.utils.patterns. That way we keep the consistency in the way we read wheel file names

This comment has been minimized.

Copy link
@edwardgeorge

edwardgeorge Jan 22, 2020

Author Contributor

aha, thanks. I've updated to use wheel_file_re.

The previous regexp was only taking the first integer of the version number,
this presented problems when the major version number reached double digits.

Poetry would determine that the version of the dependency is '1', rather than,
ie: '14'. This caused failures to solve versions.
@edwardgeorge edwardgeorge force-pushed the edwardgeorge:master branch from 8887079 to 11f6572 Jan 22, 2020
@edwardgeorge edwardgeorge requested review from sdispater and finswimmer Jan 22, 2020
Copy link
Member

sdispater left a comment

Looks good to me. Thanks a lot!

@sdispater sdispater merged commit 930515b into python-poetry:master Jan 22, 2020
16 checks passed
16 checks passed
Linting
Details
Linux (2.7)
Details
Linux (3.5)
Details
Linux (3.6)
Details
Linux (3.7)
Details
Linux (3.8)
Details
MacOS (2.7)
Details
MacOS (3.5)
Details
MacOS (3.6)
Details
MacOS (3.7)
Details
MacOS (3.8)
Details
Windows (2.7)
Details
Windows (3.5)
Details
Windows (3.6)
Details
Windows (3.7)
Details
Windows (3.8)
Details
MrGreenTea added a commit to MrGreenTea/poetry that referenced this pull request Feb 14, 2020
The previous regexp was only taking the first integer of the version number,
this presented problems when the major version number reached double digits.

Poetry would determine that the version of the dependency is '1', rather than,
ie: '14'. This caused failures to solve versions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.