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

version: fix local label comparison #379

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

version: fix local label comparison #379

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
64446f9
version: fix local label comparison
abn May 26, 2022
c4bb9bd
versions: preserve any local label when bumping
abn May 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions src/poetry/core/semver/version.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,6 @@ def allows(self, version: Version | None) -> bool:
# allow weak equality to allow `3.0.0+local.1` for `3.0.0`
if not _this.is_local() and _other.is_local():
_other = _other.without_local()
elif _this.is_local() and not _other.is_local():
_this = _this.without_local()

# allow weak equality to allow `3.0.0-1` for `3.0.0`
if not _this.is_postrelease() and _other.is_postrelease():
Expand Down
9 changes: 9 additions & 0 deletions src/poetry/core/semver/version_range.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,11 @@ def is_simple(self) -> bool:

def allows(self, other: Version) -> bool:
if self._min is not None:
if self._min.is_local() and (
not other.is_local() or self._min.local != other.local
Copy link
Member

@radoering radoering May 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

That's not completely correct. It actually has to be something like self._min.local < other.local. (However, this falls too short.)

Copy link
Member Author

@abn abn May 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

IIRC, local labels are to be treated as exact strings, so they should not be ordered when comparing.

Copy link
Member Author

@abn abn May 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

image

🤣

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it might not be suitable for stuff like 1.10.0+cu113 and 1.10.0+cpu but that's what PEP440 says.

):
return False

if other < self._min:
return False

Expand All @@ -81,6 +86,10 @@ def allows(self, other: Version) -> bool:
if not _this.is_local() and _other.is_local():
# allow weak equality to allow `3.0.0+local.1` for `<=3.0.0`
_other = _other.without_local()
elif _this.is_local() and (
not _other.is_local() or _this.local != _other.local
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

):
return False

if not _this.is_postrelease() and _other.is_postrelease():
# allow weak equality to allow `3.0.0-1` for `<=3.0.0`
Expand Down
12 changes: 9 additions & 3 deletions src/poetry/core/version/pep440/version.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ def next_major(self: T) -> T:
release = self.release
if self.is_stable() or Release(self.release.major, 0, 0) < self.release:
release = self.release.next_major()
return self.__class__(epoch=self.epoch, release=release)
return self.__class__(epoch=self.epoch, release=release, local=self.local)

def next_minor(self: T) -> T:
release = self.release
Expand All @@ -214,12 +214,13 @@ def next_minor(self: T) -> T:
or Release(self.release.major, self.release.minor, 0) < self.release
):
release = self.release.next_minor()
return self.__class__(epoch=self.epoch, release=release)
return self.__class__(epoch=self.epoch, release=release, local=self.local)

def next_patch(self: T) -> T:
return self.__class__(
epoch=self.epoch,
release=self.release.next_patch() if self.is_stable() else self.release,
local=self.local,
)

def next_prerelease(self: T, next_phase: bool = False) -> PEP440Version:
Expand All @@ -228,7 +229,9 @@ def next_prerelease(self: T, next_phase: bool = False) -> PEP440Version:
pre = self.pre.next_phase() if next_phase else self.pre.next()
else:
pre = ReleaseTag(RELEASE_PHASE_ID_ALPHA)
return self.__class__(epoch=self.epoch, release=self.release, pre=pre)
return self.__class__(
epoch=self.epoch, release=self.release, pre=pre, local=self.local
)

def next_postrelease(self: T) -> T:
if self.is_postrelease():
Expand All @@ -242,6 +245,7 @@ def next_postrelease(self: T) -> T:
pre=self.pre,
dev=self.dev,
post=post,
local=self.local,
)

def next_devrelease(self: T) -> T:
Expand All @@ -256,13 +260,15 @@ def next_devrelease(self: T) -> T:
pre=self.pre,
post=self.post,
dev=dev,
local=self.local,
)

def first_prerelease(self: T) -> T:
return self.__class__(
epoch=self.epoch,
release=self.release,
pre=ReleaseTag(RELEASE_PHASE_ID_ALPHA),
local=self.local,
)

def replace(self: T, **kwargs: Any) -> T:
Expand Down
39 changes: 39 additions & 0 deletions tests/semver/test_parse_constraint.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,45 @@
include_min=True,
),
),
(
"^1.10+cu113",
VersionRange(
min=Version.from_parts(1, 10, local="cu113"),
max=Version.from_parts(2, 0, local="cu113"),
include_min=True,
),
),
(
"~1.10.0+cu113",
VersionRange(
min=Version.from_parts(1, 10, 0, local="cu113"),
max=Version.from_parts(1, 11, 0, local="cu113"),
include_min=True,
),
),
(
">=1.10.0+cu113",
VersionRange(
min=Version.from_parts(1, 10, 0, local="cu113"),
include_min=True,
),
),
(
">=1.10.0+cu113,<2.0",
VersionRange(
min=Version.from_parts(1, 10, 0, local="cu113"),
max=Version.from_parts(2, 0),
include_min=True,
),
),
(
">=1.10.0+cu113,<2.0+cu113",
VersionRange(
min=Version.from_parts(1, 10, 0, local="cu113"),
max=Version.from_parts(2, 0, local="cu113"),
include_min=True,
),
),
],
)
@pytest.mark.parametrize(("with_whitespace_padding",), [(True,), (False,)])
Expand Down
3 changes: 2 additions & 1 deletion tests/semver/test_version.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,8 @@ def test_allows_with_local() -> None:
assert not v.allows(Version.parse("1.3.3"))
assert not v.allows(Version.parse("1.2.3-dev"))
assert not v.allows(Version.parse("1.2.3+build.2"))
assert v.allows(Version.parse("1.2.3-1"))
assert not v.allows(Version.parse("1.2.3-1"))
assert v.allows(Version.parse("1.2.3-1+build.1"))
assert v.allows(Version.parse("1.2.3-1+build.1"))
Comment on lines +131 to 132
Copy link
Member

Choose a reason for hiding this comment

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

Duplicated line.

Maybe, it's worth adding assert not v.allows(Version.parse("1.2.3")) since that's new.



Expand Down
42 changes: 29 additions & 13 deletions tests/semver/test_version_range.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,31 +110,42 @@ def test_allows_post_releases_with_post_and_local_min() -> None:
two = Version.parse("3.0.0-1")
three = Version.parse("3.0.0-1+local.1")
four = Version.parse("3.0.0+local.2")
five = Version.parse("4.0.0+local.2")

assert VersionRange(min=one, include_min=True).allows(two)
assert not VersionRange(min=one, include_min=True).allows(two)
assert VersionRange(min=one, include_min=True).allows(three)
assert VersionRange(min=one, include_min=True).allows(four)
assert not VersionRange(min=one, include_min=True).allows(four)
assert not VersionRange(min=one, include_min=True).allows(five)

assert not VersionRange(min=two, include_min=True).allows(one)
assert VersionRange(min=two, include_min=True).allows(three)
assert not VersionRange(min=two, include_min=True).allows(four)
assert VersionRange(min=two, include_min=True).allows(five)

assert not VersionRange(min=three, include_min=True).allows(one)
assert not VersionRange(min=three, include_min=True).allows(two)
assert not VersionRange(min=three, include_min=True).allows(four)
assert not VersionRange(min=three, include_min=True).allows(five)

assert not VersionRange(min=four, include_min=True).allows(one)
assert VersionRange(min=four, include_min=True).allows(two)
assert VersionRange(min=four, include_min=True).allows(three)
assert not VersionRange(min=four, include_min=True).allows(two)
assert not VersionRange(min=four, include_min=True).allows(three)
assert VersionRange(min=four, include_min=True).allows(five)

assert not VersionRange(min=five, include_max=True).allows(one)
assert not VersionRange(min=five, include_max=True).allows(two)
assert not VersionRange(max=five, include_max=True).allows(three)
assert not VersionRange(min=five, include_max=True).allows(four)


def test_allows_post_releases_with_post_and_local_max() -> None:
one = Version.parse("3.0.0+local.1")
two = Version.parse("3.0.0-1")
three = Version.parse("3.0.0-1+local.1")
four = Version.parse("3.0.0+local.2")
five = Version.parse("4.0.0+local.2")

assert VersionRange(max=one, include_max=True).allows(two)
assert not VersionRange(max=one, include_max=True).allows(two)
assert VersionRange(max=one, include_max=True).allows(three)
assert not VersionRange(max=one, include_max=True).allows(four)

Expand All @@ -143,12 +154,17 @@ def test_allows_post_releases_with_post_and_local_max() -> None:
assert VersionRange(max=two, include_max=True).allows(four)

assert VersionRange(max=three, include_max=True).allows(one)
assert VersionRange(max=three, include_max=True).allows(two)
assert VersionRange(max=three, include_max=True).allows(four)
assert not VersionRange(max=three, include_max=True).allows(two)
assert not VersionRange(max=three, include_max=True).allows(four)

assert not VersionRange(max=four, include_max=True).allows(one)
assert not VersionRange(max=four, include_max=True).allows(two)
assert not VersionRange(max=four, include_max=True).allows(three)

assert VersionRange(max=four, include_max=True).allows(one)
assert VersionRange(max=four, include_max=True).allows(two)
assert VersionRange(max=four, include_max=True).allows(three)
assert not VersionRange(max=five, include_max=True).allows(one)
assert not VersionRange(max=five, include_max=True).allows(two)
assert not VersionRange(max=five, include_max=True).allows(three)
assert VersionRange(max=five, include_max=True).allows(four)


@pytest.mark.parametrize(
Expand All @@ -161,9 +177,9 @@ def test_allows_post_releases_with_post_and_local_max() -> None:
id="post",
),
pytest.param(
Version.parse("3.0.0"),
Version.parse("3.0.0+local.1"),
Version.parse("3.0.0+local.2"),
Version.parse("3.0.0-1+local.1"),
Version.parse("3.0.0-2+local.1"),
id="local",
),
],
Expand Down Expand Up @@ -192,7 +208,7 @@ def test_allows_post_releases_explicit_with_max(
pytest.param(
Version.parse("3.0.0"),
Version.parse("3.0.0+local.1"),
Version.parse("3.0.0+local.2"),
Version.parse("3.0.0-1+local.1"),
id="local",
),
],
Expand Down