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

semver/pep508: handle wildcard exclusions #343

Merged

Conversation

abn
Copy link
Member

@abn abn commented May 2, 2022

Prior to this change, when exporting PEP 508 strings for dependencies, wildcard exclusion constraints like !=2.0.* were incorrectly serialised as invalid PEP 508 due to how version unions were serialsed.

This change allows for determining if a version union is a single wildcard range exclusion, and if so serialise it appropriately.

Resolves: python-poetry/poetry#5470

PS: Not entirely sure if this is the best fix, but it does the trick for now. Not entirely happy with the determination logic, if anyone has a better approach please do share.

Additionally, during testing it was identified that the X_CONSTRAINT pattern mattaching when parsing constraints ignores any non semver versions (eg: calver). While the parser is in the "semver" package, since Poetry needs to support PEP440 based constraints, this should be improved. Will propose a fix in another PR.

@abn
Copy link
Member Author

abn commented May 2, 2022

@radoering since you looked at the last semver fix; can you take a peek here too?

@abn abn requested a review from a team May 2, 2022 21:03
Copy link
Member

@radoering radoering left a comment

Choose a reason for hiding this comment

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

Seems a bit cumbersome indeed, but since I have no better approach I think it's fine for now with some minor changes.

src/poetry/core/packages/dependency.py Show resolved Hide resolved
src/poetry/core/semver/version_union.py Outdated Show resolved Hide resolved
tests/packages/test_dependency.py Outdated Show resolved Hide resolved
src/poetry/core/semver/version_union.py Outdated Show resolved Hide resolved
@abn abn force-pushed the fix-single-version-range-exclusion branch from 43d5d27 to 663c54d Compare May 5, 2022 11:05
@abn abn requested a review from radoering May 5, 2022 12:53
@abn abn force-pushed the fix-single-version-range-exclusion branch 3 times, most recently from ecd9435 to 00193c8 Compare May 6, 2022 14:17
@abn abn dismissed radoering’s stale review May 6, 2022 14:17

Resolved review feedback.

Copy link
Member

@radoering radoering left a comment

Choose a reason for hiding this comment

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

Wow, definitively more extensive than I would have expected. But since it is encapsulated in its own method and you already know the goal of the long else branch after reading the short if branch it is not that difficult to follow. And since the test coverage seems fine, I'm fine with it for now.

Just two minor nitpicks (see comments).

src/poetry/core/packages/dependency.py Show resolved Hide resolved
src/poetry/core/semver/version_union.py Outdated Show resolved Hide resolved
@abn abn force-pushed the fix-single-version-range-exclusion branch from 00193c8 to acb2336 Compare May 6, 2022 16:32
@abn abn requested a review from radoering May 6, 2022 16:33
Prior to this change, when exporting PEP 508 strings for dependencies,
wildcard exclusion constraints like `!=2.0.*` were incorrectly
serialised as invalid PEP 508 due to how version unions were serialsed.

This change allows for determining if a version union is a single
wildcard range exclusion, and if so serialise it appropriately.

Resolves: python-poetry/poetry#5470
@abn abn force-pushed the fix-single-version-range-exclusion branch from acb2336 to 2875257 Compare May 6, 2022 16:35
@sonarcloud
Copy link

sonarcloud bot commented May 6, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
1.5% 1.5% Duplication

@abn abn dismissed radoering’s stale review May 6, 2022 16:36

Resolved review feedback.

@radoering radoering merged commit be79d5e into python-poetry:master May 6, 2022
@abn abn deleted the fix-single-version-range-exclusion branch May 6, 2022 17:51
@finswimmer finswimmer mentioned this pull request May 20, 2022
bostonrwalker pushed a commit to bostonrwalker/poetry-core that referenced this pull request Aug 29, 2022
Prior to this change, when exporting PEP 508 strings for dependencies,
wildcard exclusion constraints like `!=2.0.*` were incorrectly
serialised as invalid PEP 508 due to how version unions were serialised.

This change allows for determining if a version union is a single
wildcard range exclusion, and if so serialise it appropriately.

Resolves: python-poetry/poetry#5470
DavidVujic pushed a commit to DavidVujic/poetry-core that referenced this pull request Aug 31, 2022
Prior to this change, when exporting PEP 508 strings for dependencies,
wildcard exclusion constraints like `!=2.0.*` were incorrectly
serialised as invalid PEP 508 due to how version unions were serialised.

This change allows for determining if a version union is a single
wildcard range exclusion, and if so serialise it appropriately.

Resolves: python-poetry/poetry#5470
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.

>= plus != generates invalid Requires-Dist
3 participants