Repository navigation
Conversation
There was a problem hiding this comment.
Hi @masonjian, thanks for picking up #262! Really appreciate your contribution! ❤️ The tests and docs are thorough.
Unfortunately, I don't think this PR addresses the original issue. The motivating example is choosing a bump on a staging branch. If staging is 1.0.1-rc.1 and master is 1.0.1, the answer should be bump_minor(). Here, Version.parse("1.0.1-rc.1").diff(Version.parse("1.0.1")) returns 'prerelease', which would point users to bump_prerelease().
I'd like to propose a different direction: make diff match node-semver's diff. Node's semver.diff (checked against semver@7.7.2) differs in ways people will likely notice:
v1 → v2 |
node semver.diff |
This PR |
|---|---|---|
1.2.3+one → 1.2.3+two |
null |
build |
1.2.3-alpha → 1.2.3 |
patch |
prerelease |
1.2.3 → 1.2.4-rc.1 |
prepatch |
patch |
Node returns a bump type with a pre prefix for prerelease targets, ignores build metadata, and is direction-aware.
Python users who know node-semver would then get the same answers, and the function would return a bump type rather than a field name. Node's algorithm ports cleanly:
def diff(self, other: Comparable) -> str | None:
# operand coercion as in the current version
cmp = self.compare(other)
if cmp == 0:
return None
high, low = (self, other) if cmp > 0 else (other, self)
high_pre = bool(high.prerelease)
low_pre = bool(low.prerelease)
if low_pre and not high_pre:
if not low.minor and not low.patch:
return "major"
if low.to_tuple()[:3] == high.to_tuple()[:3]:
return "minor" if low.minor and not low.patch else "patch"
prefix = "pre" if high_pre else ""
for part in ("major", "minor", "patch"):
if getattr(self, part) != getattr(other, part):
return prefix + part
return "prerelease"What this changes:
- Build-only differences return None, because
compare()ignores build metadata. This matches node and==, so the 'build' return value goes away. - The result depends on which version is higher, matching node's behavior.
- The return values are
major,minor,patch,premajor,preminor,prepatch,prerelease, orNone.
Node's quirks come along. For example, 1.0.0-alpha → 1.0.0 returns major, and 1.0.1-rc.1 → 1.0.1 returns patch.
This still doesn't solve the staging case from #262, since node returns patch there too. If you want that answer, it would need a separate, explicitly named method, or docs showing how to combine diff with a core comparison.
This is a rewrite of the method and its tests rather than a patch. The tests could be based on node-semver's own diff test cases, which would give good coverage and a clear parity check. The docs would also need to change from "first differing part" to "bump type, matching node-semver".
Happy to review a revision, and it may be worth agreeing on the direction in #262 first.
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "other", [None, 1, "1.2.3", b"1.2.3", (1, 2, 3), [1, 2, 3], {"major": 1}] |
There was a problem hiding this comment.
This test currently asserts that "1.2.3", b"1.2.3", (1, 2, 3), [1, 2, 3], and {"major": 1} raise TypeError. Those should now be accepted, so move them into a positive test. Keep None, 1, and similar invalid types in the rejection test.
There was a problem hiding this comment.
Moved the supported input types into positive conversion tests in b89678f, including full prerelease/build fields and incomplete collection inputs. The rejection test now keeps None, 1, and other unsupported types.
| Added ``Version.diff(other)`` to identify the first differing version part, | ||
| including build metadata, independently of comparison direction. It returns | ||
| ``None`` when all parts are identical. |
There was a problem hiding this comment.
If we want to follow my suggestion, we would need to adjust the wording:
| Added ``Version.diff(other)`` to identify the first differing version part, | |
| including build metadata, independently of comparison direction. It returns | |
| ``None`` when all parts are identical. | |
| Added :meth:`~semver.version.Version.diff` to return the type of bump needed to go from one version to another, matching the ``semver.diff`` function in node-semver. It returns ``"major"``, ``"minor"``, ``"patch"``, ``"premajor"``, ``"preminor"``, ``"prepatch"``, or ``"prerelease"``, or ``None`` when the versions have the same precedence. Build metadata is ignored. |
There was a problem hiding this comment.
Updated the changelog in b89678f with all seven node-semver 7.7.2 classifications, None for equal precedence, and ignored build metadata. I used "classify valid version pairs" and explicitly noted that staging-branch bump policy is outside this change, following the scope agreed in #262.
Related to #262. This follows the maintainer-confirmed scope and does not fully resolve the issue.
Add Version.diff(other) classification matching node-semver 7.7.2 for valid version pairs:
This is classification only. next_version, bump behavior, and staging policy are unchanged.
Validation on macOS with Python 3.12.13:
The full tox multi-Python/OS matrix and optional native backend were not run locally. At the latest check, GitHub's Python and CodeQL workflows awaited maintainer approval.
Independent review found no blocking issues. AI assistance was used for implementation, automated testing, and review.