-
Notifications
You must be signed in to change notification settings - Fork 96
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
Comments
Thanks @Learloj for your issue. I think we need to distinguish between bumping and comparing. 🙂
I've looked into node's semver library and this is a short shell session what they do:
Look at the last two lines. I was also surprised how they bump a prerelease. They just append |
Hi @tomschr ! Kudos for your fast reply! That does not happen often 😁 I think it is a very elegant way to resolve this indeed:
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. |
Great, thank you @Learloj! 👍
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! ❤ |
@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:
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:
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? |
Hi @Learloj, Correcting the implementation of Just a crazy idea and I'd like your opinion on that. What about if we introduce an >>> 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 So to summarize it, I see these options:
Perhaps some could be even combined. What do you think would be the best option here? My only concerns are that this change is:
|
@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 Regarding the 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 If this is not desired, then I think deprecating the Let me know what you prefer, I am fine with creating a PR for either solution. |
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.
Yes, that's pretty similar. It was just an idea. Or we could sell that as an alias to
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
Maybe that should be the preferred way anyway, independent from this PR. Thank you for all your ideas! ❤ That was very valuable. 👍 |
…a newer version, and raising an empty prerelease version has the option to raise the patch version as well
@tomschr please have a look at my PR and provide any and all feedback (don't hold back :-) ). |
…a newer version, and raising an empty prerelease version has the option to raise the patch version as well
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
Expected behavior
Not entirely sure how the bump_prerelease version should behalve when it gets
rc9
. ASCII-wise, after9
you could considera
, but you getrca
,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 increaserc9
to a higher value.Environment
No response
The text was updated successfully, but these errors were encountered: