Skip to content

semver.bump_prerelease on *-rc9 results in lower version #460

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

Closed
Learloj opened this issue Jan 22, 2025 · 8 comments · Fixed by #462
Closed

semver.bump_prerelease on *-rc9 results in lower version #460

Learloj opened this issue Jan 22, 2025 · 8 comments · Fixed by #462
Assignees
Labels
Bug Error, flaw or fault to produce incorrect or unexpected results

Comments

@Learloj
Copy link
Contributor

Learloj commented Jan 22, 2025

Which version of Python is the problem with?

3.8

What semver version are you using?

No response

What OS are you using? (Add more in the Environment section)

macOS

Situation

When calling semver.bump_prerelease on a version with a prerelease of -rc9, it gets bumped to -rc10. However. according to the semver spec (https://semver.org/#spec-item-11 section 11.4 items 1+2), 'rc9' is considered a string and will be compared to 'rc10' using ASCII sort order, which considers 'rc10' to be lower than 'rc9'.

How to reproduce

>>> import semver
>>> version1 = '1.0.0-rc9'
>>> version2 = semver.bump_prerelease(version1)
>>> version2
'1.0.0-rc10'
>>> semver.compare(version1, version2)
1

Expected behavior

Not entirely sure how the bump_prerelease version should behalve when it gets rc9. ASCII-wise, after 9 you could consider a, but you get rca, rcb which feels a bit odd. Another option could be that the function just returns some kind of error, indicating it cannot determine how to increase rc9 to a higher value.

Environment

No response

@tomschr tomschr added the Bug Error, flaw or fault to produce incorrect or unexpected results label Jan 22, 2025
@tomschr
Copy link
Member

tomschr commented Jan 22, 2025

Thanks @Learloj for your issue.

I think we need to distinguish between bumping and comparing. 🙂

Not entirely sure how the bump_prerelease version should behalve when it gets rc9.

I've looked into node's semver library and this is a short shell session what they do:

$ npm install semver
added 1 package in 562ms
$ node
Welcome to Node.js v20.18.1.
Type ".help" for more information.
> const semver = require('semver');
undefined
> v = semver.parse("2.3.4-rc9");
SemVer {
  options: {},
  loose: false,
  includePrerelease: false,
  raw: '2.3.4-rc9',
  major: 2,
  minor: 3,
  patch: 4,
  prerelease: [ 'rc9' ],
  build: [],
  version: '2.3.4-rc9'
}
> semver.inc(v, "prerelease")
'2.3.4-rc9.0'

Look at the last two lines. I was also surprised how they bump a prerelease. They just append .0 and bump that. Would that be an option?

@Learloj
Copy link
Contributor Author

Learloj commented Jan 22, 2025

Hi @tomschr ! Kudos for your fast reply! That does not happen often 😁

I think it is a very elegant way to resolve this indeed:

  • Split the pre release tag by dots and consider the last value
  • If the value is numeric, increase it
  • If not, add .0

That way it will always be considered a higher version according to the semver spec.

If it is ok, I don't mind working on a PR to implement this.

@tomschr
Copy link
Member

tomschr commented Jan 22, 2025

Great, thank you @Learloj! 👍

If it is ok, I don't mind working on a PR to implement this.

Absolutely! I'd love to get some support. 😍

To make it hopefully easier for you, I've described all the details at the Contributing to semver section. I think you only need to consider the sub sections 6.2, 6.3, and 6.4.

Thank you! ❤

@Learloj
Copy link
Contributor Author

Learloj commented Jan 25, 2025

@tomschr while working on improving the tests to cover this issue, I ran into another possible bug. When bumping the pre release version on a version that has no prerelease part yet, semver only adds the prerelease. As a version without a prerelease is considered newer than the same version with a prerelease, we basically get a version that is lower than the version which we bumped, which feels wrong to me.

To demonstrate:

>>> version = semver.Version.parse('1.2.3')
>>> newversion = version.bump_prerelease()
>>> newversion
Version(major=1, minor=2, patch=3, prerelease='rc.1', build=None)
>>> version.compare(newversion)
1

Checking npm, I see that when you bump a prerelease on a version without a prerelease part, they also bump the minor version to ensure the resulting version is considered higher:

> v = semver.parse("2.3.4");
SemVer {
  options: {},
  loose: false,
  includePrerelease: false,
  raw: '2.3.4',
  major: 2,
  minor: 3,
  patch: 4,
  prerelease: [],
  build: [],
  version: '2.3.4'
}
> semver.inc(v, "prerelease")
'2.3.5-0'

How shall I proceed with this? Keep the current implementation for this (and not add a test to validate if the outcome of the bump is higher than the input), or shall I change the behavior and also increase the minor in such cases?

@tomschr
Copy link
Member

tomschr commented Jan 26, 2025

Hi @Learloj,
thanks for all your efforts. 👍 Yes, unfortunately the .bump_prerelease() method has this problem. I didn't change that due to backward compatibility reasons. To mitigate this problem, use the .next_version() method.

Correcting the implementation of .bump_prerelease() would introduce a backward incompatible change which I'd like to avoid. If it's necessary we could do that, but we would have to increase the major version to 4. 🤔

Just a crazy idea and I'd like your opinion on that. What about if we introduce an .inc() method similar to the NPM version? This could work like this:

>>> version = semver.Version.parse('1.2.3')
>>> version.inc(part="prerelease")
Version(major=1, minor=2, patch=3, prerelease='0', build=None)

The implementation works in the same way as semver's NPM version. The next step would be to deprecate all the bump_*() versions. There should be only one way. If wanted, we could add shortcuts like .inc_major(), inc_minor()` etc.

So to summarize it, I see these options:

  1. Make a semver 4 release and fix the .bump_prerelease() method (and maybe others).
  2. Deprecate all .bump_*() methods and propagate .next_version() as the preferred method.
  3. Introduce an .inc() method similar to NPM and deprecate all .bump_*() methods.

Perhaps some could be even combined. What do you think would be the best option here?

My only concerns are that this change is:

  1. Backward compatible, or
  2. If we introduce a backward incompatible change, we raise the major version and document the migration path.

@Learloj
Copy link
Contributor Author

Learloj commented Jan 26, 2025

@tomschr fair point. I think it's a bit confusing that we have multiple ways to 'increase' the version, which behave differently. I also think that adding a new method, even if we'd deprecate the others, might create more confusion, plus it'd affect a lot more people who'd need to adjust their code to step away from the bump_* methods.

Regarding the inc method, isn't that exactly what next_version() currently does? I checked the implementations, and both increase the patch version if there was no prerelease set (though npm adds 0 and semver adds rc.1 as prerelease).

I agree with your statement regarding backwards compatibility, but this change also feels so small for me that it's a bit drastic to increase the major version of semver. What we could do, is introduce an argument to bump_prerelease() named something like ensure_newer_version or bump_patch_when_empty or some other logical name, which would default to false. That way, we keep backwards compatibility, and when someone sets it to true, you'd get the 'expected' behaviour which bumps the patch version if there was no prerelease version set, resulting in a newer version. Then later, when there are other bigger changes and semver gets bumped to major version 4, we could consider adjusting the default value of this argument to true.

If this is not desired, then I think deprecating the bump_*() methods in favour of next_version() would not be a bad idea, as they both (should) do the same thing. I don't think it's necessary to add a inc() method, unless I am missing a big difference between what next_version() does, and what inc() in NPM does.

Let me know what you prefer, I am fine with creating a PR for either solution.

@tomschr
Copy link
Member

tomschr commented Jan 26, 2025

@Learloj

I think it's a bit confusing that we have multiple ways to 'increase' the version, which behave differently

Yes, was probably a bad idea. If I recall correctly, it was for historical reasons and to maintaining backward compatibility. But maybe it would be better/easier/cleaner if we have only one method that works for all cases.

Regarding the inc method, isn't that exactly what next_version() currently does?

Yes, that's pretty similar. It was just an idea. Or we could sell that as an alias to next_version() for people knowing or coming from the NPM semver library. Anyway, that's probably a different topic and not relevant for now.

What we could do, is introduce an argument to bump_prerelease() named something like ensure_newer_version or bump_patch_when_empty or some other logical name, which would default to false

Ahh, what a nice idea! I like it! Soo... I'd say, go for that. 👍

I'm not so sure about the naming of the optional argument. I have a slight preference of bump_when_empty. But if you have an idea to make it shorter, let us know.

If this is not desired, then I think deprecating the bump_*() methods in favour of next_version() would not be a bad idea, [...]

Maybe that should be the preferred way anyway, independent from this PR.

Thank you for all your ideas! ❤ That was very valuable. 👍

Learloj pushed a commit to Learloj/python-semver that referenced this issue Jan 26, 2025
…a newer version, and raising an empty prerelease version has the option to raise the patch version as well
@Learloj
Copy link
Contributor Author

Learloj commented Jan 26, 2025

@tomschr please have a look at my PR and provide any and all feedback (don't hold back :-) ).

tomschr pushed a commit to Learloj/python-semver that referenced this issue Jan 28, 2025
…a newer version, and raising an empty prerelease version has the option to raise the patch version as well
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Error, flaw or fault to produce incorrect or unexpected results
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants