Fixes unstable next breaking version when major is 0

[mazinesy]

Resolves: python-poetry/poetry#6519

• Added tests for changed code.
• Updated documentation for changed code.

What this PR contains:

• moves Version.stable to PEP440Version.stable as Version.stable was breaking the precision half of the time.
• adds bunch of test
• fixes Version.next_breaking to allow for version with major == 0

Extra info

It would be great to have to have a poetry build step that checks the requires_dist output against poetry to make sure the package is actually usable.

[dimbleby]

This seems a reasonable interpretation of next_breaking() to me, as well as apparently being what poetry 1.1 used to do. On the other hand the unfixed interpretation also seems reasonable, so I'd welcome maintainer opinion...

Per python-poetry/poetry#6519 (comment), it's not clear that this is really the problem that you wanted to solve anyway.

[dimbleby]

on further investigating PEP440 I no longer think that the unfixed interpretation is reasonable

"The exclusive ordered comparison <V MUST NOT allow a pre-release of the specified version unless the specified version is itself a pre-release"

So eg >=0.0.2a1,<0.0.2 is actually empty (since the second part explicitly excludes the pre-releases).

That doesn't seem like a useful meaning of ^0.0.2a1 at all

So this looks good to me

[radoering]

There are still some cases that might not be correct:

^0.1 becomes >=0.1,<0.2

but

^0.1-alpha.1 becomes >=0.1-alpha.1,<0.1.1

That's probably not expected, is it?

Can you add a dedicated unit test for next_breaking in tests/semver/test_version.py similar to the tests in tests/pep440/test_version.py for the next_... methods of PEP440Version, please?

[dimbleby]

good spot.

There's more than one way to skin a cat, but I'll re-suggest python-poetry/poetry#6519 (comment)

suggest that next_breaking() wants to start by going stable = self.stable and then consider stable everywhere it currently considers self.

And add a testcase in here - next_breaking() has no coverage at the moment so the more the merrier, cf the tests for next_major() and next_minor() and suchlike.

[mazinesy]

There are still some cases that might not be correct:

@radoering
I thought 0.2-alpha1 was not a valid version, but looking through PEP440, it is. I will add the test cases for this and also will add test cases for the Version class. I thought it was weird we had no tests for that class and everything was in helper.

There's also a general issue where Version.next_breaking() is keeping the precision. There's a test for this. But Version.stable breaks the versioning.

Version is in the semver package which would imply it represents a semver version. But, Version.next_breaking allows for non semver versions even if it is in the SemverVersion class. Here are my thoughts:

1. Version.stable breaks the version precision when the version is unstable but does not break it when it is stable. That is a weird behaviour. We probably should always break version precision when using Version.stable instead of doing both 🤔
2. Version.next_breaking() currently does not break the precision, but should break it. Since ^ is not a valid PEP440 constraint, when someone asks for ^1.1, it makes sense to basically say, evaluate ^1.1.0

[mazinesy]

My last comment feels a bit too much opinionated so I kept the precision in next_breaking(), but maybe, next_breaking() should live in PEP440Version as it keeps precision

[dimbleby]

I appreciate all the tests! If anything I'd say you've gone a little over the top here, but I shouldn't discourage tests!

I have some questions

• why move the implementation of stable()? I don't think this was needed
  • I'm not sure that I mind very much, it just seems unnecessary to mix this in with the fix you're making
• you've changed the behaviour so that next_breaking() never does a patch bump, which it used to do for 0.0.x. Why?
  • Again I'm not sure what I think of this, if it had been that way in the first place I likely wouldn't have thought twice about it
  • it seems like an unnecessary change so far as Breaking change in handling of some version constraints from poetry-core 1.0.0 to 1.1.0 poetry#6519 is concerned

I'd also suggest that a simpler fix to make stable() retain precision is:

         if self.is_stable():
             return self

-        return self.next_patch()
+        stable = Version(release=self.release, epoch=self.epoch)
+        return stable

[dimbleby]

oh also I now notice that you've changed the implementation of stable() so that it sometimes returns a postrelease. (I was blind to this before on account of it moving between classes).

Again this seems like a change that is somewhere between not-needed-right-now and I-dont-even-know-whether-its-right.

Edit: it probably is right, if 1.2.3.post4 -> 1.2.3.post4 and not to 1.2.3, then so should 1.2.3.post4.dev5.

(my suggested reimplementation of stable() still handles this compactly, just add self.post to the arguments)

[mazinesy]

Here are some example of what stable used to do:

version => stable
0.2 => 0.2
0.2-a1 => 0.2.1 (adds precision)
1 => 1
1-alpha.1 => 1.0.1 (adds precision)

Very weird behaviour and it was used in next_breaking().

The reason why next_breaking never does a patch bump for 0 majors comes from a test that described the expected behaviour here. Plus, this is the current behaviour on main. But I see that this example did not have a 0 as minor, that's where my problem is. I'll fix that.

So the expected behavior's for next_breaking are:

0 => 1
0.1 => 0.2
0.0.1 => 0.0.2 (minor is also 0)
0.1.2 => 0.2.0 (minor is not 0)

Adding -alpha.x does not change that behavior.

[dimbleby]

We're talking past each other. I've no problem with stable() being fixed to preserve precision. I am (i) asking why it moved to a different class and (ii) suggesting a simpler implementation.

[mazinesy]

We're talking past each other. I've no problem with stable() being fixed to preserve precision. I am (i) asking why it moved to a different class and (ii) suggesting a simpler implementation.

(i)
The main reason being that Version represents a SemVer version which must be in X.Y.Z. Currently, doing Version.parse("0.1") yields Version<0.1> which is an invalid semver version as semver always has a precision of 3. My logic is, everything that deals with preserving versions should live in PEP440Version version and not the semverVersion. From this, the current implementation of next_breaking() should also live in PEP440Version

(ii) I just implemented it

[dimbleby]

poetry-core's Version does not always have a precision of three. So moving this code out of the Version class for that reason is unnecessary.

I propose that the entire diff in src should look like this:

diff --git a/src/poetry/core/semver/version.py b/src/poetry/core/semver/version.py
index 2fe6fe8..4746121 100644
--- a/src/poetry/core/semver/version.py
+++ b/src/poetry/core/semver/version.py
@@ -32,21 +32,17 @@ class Version(PEP440Version, VersionRangeConstraint):
         if self.is_stable():
             return self

-        return self.next_patch()
+        post = self.post if self.pre is None else None
+        return Version(release=self.release, post=post, epoch=self.epoch)

     def next_breaking(self) -> Version:
-        if self.major == 0:
-            if self.minor is not None and self.minor != 0:
-                return self.next_minor()
+        if self.major > 0 or self.minor is None:
+            return self.stable.next_major()

-            if self.precision == 1:
-                return self.next_major()
-            elif self.precision == 2:
-                return self.next_minor()
+        if self.minor > 0 or self.patch is None:
+            return self.stable.next_minor()

-            return self.next_patch()
-
-        return self.stable.next_major()
+        return self.stable.next_patch()

your tests for stable() will then want moving (because they go with the Version and not the PEP440Version). And I think that will expose some differences eg 1.2.3.4dev0.stable() will be 1.2.3.4 - which seems correct.

[mazinesy]

your tests for stable() will then want moving (because they go with the Version and not the PEP440Version. And I think that will expose some differences eg 1.2.3.4dev0.stable() will be 1.2.3.4 - which seems correct.

Alright, let me move things around. Btw, SonarCloud is freaking out because 1.2.3.4 looks like an IP address.

I'll restate my concern about the following even if this PR won't deal with it.

@dataclasses.dataclass(frozen=True)
class Version(PEP440Version, VersionRangeConstraint):
    """
    A parsed semantic version number.
    """
    ...

# This should not work as `0.1` is not a valid semantic version
Version.parse('0.1').text == "0.1"

# Either, raises an error
Version.parse("0.1")

# Or
Version.parse("0.1").text == "0.1.0"

[dimbleby]

looks good to me but (i) I'm more or less reviewing my own code at this point and (ii) my review doesn't count for anything anyway

[mazinesy]

@radoering would you be so kind to rereview this PR and there seems to be an issue with SonarCloud which thinks the version 1.2.3.4 is an IP address

[radoering]

I'll restate my concern about the following even if this PR won't deal with it.

The class has some semver related methods (like next_breaking) but apart from that it's just a specific VersionConstraint representing a single version and not necessarily a semantic version number. Maybe, we should just remove the docstring because it's more confusing than helpful. Actually, semver is not even a suitable name for the package anymore, but that's something for another PR.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication