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.

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

Bumping alphanumeric identifiers #369

Open
fleetingbytes opened this issue Nov 16, 2022 · 12 comments
Open

Bumping alphanumeric identifiers #369

fleetingbytes opened this issue Nov 16, 2022 · 12 comments
Labels
Bug Error, flaw or fault to produce incorrect or unexpected results

Comments

@fleetingbytes
Copy link
Contributor

Situation

When running some tests for my semver plugin for hatch, I was surprised that if prerelease's or build's last identifier is alphanumeric, bumping does nothing.

To Reproduce

# semver 2.13.0
>>> from semver import VersionInfo
>>> v = VersionInfo.parse("1.0.0-rc")
>>> v.bump_prerelease()
VersionInfo(major=1, minor=0, patch=0, prerelease='rc', build=None)
>>> b = VersionInfo.parse("1.0.0+build")
>>> b.bump_prerelease()
VersionInfo(major=1, minor=0, patch=0, prerelease=None, build='build')

Expected Behavior

Numeric identifier 2 gets appended, in my expectation.

>>> v = VersionInfo.parse("1.0.0-rc")
>>> v.bump_prerelease()
VersionInfo(major=1, minor=0, patch=0, prerelease='rc.2', build=None)
>>> b = VersionInfo.parse("1.0.0+build")
>>> b.bump_prerelease()
VersionInfo(major=1, minor=0, patch=0, prerelease=None, build='build.2')

Environment

  • OS: Windows
  • Python version 3.10
  • Version of semver library 2.13.0
@fleetingbytes fleetingbytes added the Bug Error, flaw or fault to produce incorrect or unexpected results label Nov 16, 2022
@tomschr
Copy link
Member

tomschr commented Nov 16, 2022

@Nagidal Thank you for your report, much appreciated. 👍

I've read the original semver specification, but I haven't found a specific answer to our case. The closest is the example from item 11, subitem 4. However, it deals with comparison, not with bumping a pre-release.

Nevertheless, your concern seems to be valid. However, I'm not sure about your expectation. You wrote that you've expected 2 from 1.0.0-rc. Why not 1? Or 0?

To get some inspiration, I've looked into npm's semver, especially semver.inc. This is the little interactive session that I did with node:

> const semver = require('semver');

> v = semver.parse("1.0.0-rc");
> semver.inc(v, "prerelease");
'1.0.0-rc.0'

In their case having just a rc as pre-release and bumping it produces rc.0. I think, that is reasonable and I would follow their approach. This would mean, our Python session would look like this:

>>> v = Version.parse("1.0.0-rc")
>>> v.bump_prerelease()
Version(major=1, minor=0, patch=0, prerelease='rc.0', build=None)

What do you think?
(By the way, VersionInfo was renamed to Version. The first is deprecated and will be removed in later versions.)

@tomschr
Copy link
Member

tomschr commented Nov 16, 2022

I remember now that @vincent-herlemont opened in #344 a very similar issue (see also PR #365)

@fleetingbytes
Copy link
Contributor Author

Hello Tom,

thanks for the comparison with npm semver. I wasn't aware of their strategy. My reason for picking the 2 was partly intuition, and partly unfamiliarity with item 11, subitem 4 of the semver spec.

Intuitively, I thought that I would rank alpha.0 < alpha. And my intuitive ranking between alpha.1 and alpha was fuzzy, you could argue either way. On the other hand, it was immediately obvious to me hat alpha < alpha.2. That's why I was suggesting 2 as the added default numeric identifier.

But now, having read the specification, appending 0 makes probably the most sense. I also like the Idea that we would follow the npm semver behavior.

@fleetingbytes
Copy link
Contributor Author

As for using VersionInfo instead of Version: the 3.0.0-dev.3 is a pre-release and as such not fit for use in production yet. What you get when you pip install python-semver is the 2.13.0, so that's what I wrote my plugin for. I intend to update it to use python-semver 3.0.0 once it is released.

@tomschr
Copy link
Member

tomschr commented Nov 17, 2022

As for using VersionInfo instead of Version: the 3.0.0-dev.3 is a pre-release and as such not fit for use in production yet. What you get when you pip install python-semver is the 2.13.0, so that's what I wrote my plugin for. I intend to update it to use python-semver 3.0.0 once it is released.

Yes, that's true. It was just meant as a friendly reminder for the upcoming release. 🙂

@tomschr
Copy link
Member

tomschr commented Nov 19, 2022

Hi @Nagidal,

as I already wrote, your issue is related to #344. I gave both some thought and it raises some questions to me. I would like to hear your opinion (and maybe @vincent-herlemont too?):

  1. We could follow the NPM's approach and bump a version from 1.0.0-rc to 1.0.0-rc.0. However, the return value would be different from what semver2 did. If we follow NPM's approach, this would introduce a backward-incompatible change. As we will release semver3, that would be fine for a major release. Of course I document incompatible changes. Still not entirely sure as people could still be surprised by that.

  2. Issue Unable to create pre-release with an empty token #344 requests to pass an empty string to .bump_prerelease() (and .bump_build()). What should happen if we already have a release and want to bump it?

     >>> Version.parse("1.0.0-rc").bump_prerelease("")
     # return value -> ???

    Possible values could be 1.0.0-0, 1.0.0-rc.0, 1.0.0-rc.1. Or something else?

What do you think?

@fleetingbytes
Copy link
Contributor Author

fleetingbytes commented Nov 21, 2022

I think in these cases we could take advantage of Python's None not being the same as "". If we start counting the prereleases at 0, this is what we should go with:

Old version API call New version Comment
1.0.0 bump_prerelease() 1.0.0-rc.0 Lower version!
1.0.0 bump_prerelease("") 1.0.0-0 rc replaced with "". Lower version!
1.0.0-rc bump_prerelease() 1.0.0-rc.0
1.0.0-rc bump_prerelease("") 1.0.0-0 rc replaced with "". Lower version!
1.0.0-rc bump_prerelease("rc.1") 1.0.0-rc.1
1.0.0-rc bump_prerelease("0") 1.0.0-0 rc replaced with 0. Lower version!
1.0.0-rc bump_prerelease("1") 1.0.0-1 rc replaced with 1. Lower version!
1.0.0-alpha.3 bump_prerelease() 1.0.0-alpha.4
1.0.0-alpha.3 bump_prerelease("") 1.0.0-0 Lower version!
1.0.0-alpha.3 bump_prerelease("rc") 1.0.0-rc
1.0.0-alpha.3 bump_prerelease("rc.0") 1.0.0-rc.0
1.0.0-alpha.3 bump_prerelease("rc.1") 1.0.0-rc.1

I now prefer not to follow the npm semver behavior of bumping alphanumeric identifiers. It is very different from python-semver 2.x. I like the default numeric identifiers starting at 1 better. The user can still override it:

Old version API call New version Comment
1.0.0 bump_prerelease() 1.0.0-rc.1 Lower version!
1.0.0 bump_prerelease("") 1.0.0-1 rc replaced with "". Lower version!
1.0.0-rc bump_prerelease() 1.0.0-rc.1
1.0.0-rc bump_prerelease("") 1.0.0-1 rc replaced with "". Lower version!
1.0.0-rc bump_prerelease("rc.0") 1.0.0-rc.0
1.0.0-rc bump_prerelease("0") 1.0.0-0 rc replaced with 0. Lower version!
1.0.0-rc bump_prerelease("1") 1.0.0-1 rc replaced with 1. Lower version!
1.0.0-alpha.3 bump_prerelease() 1.0.0-alpha.4
1.0.0-alpha.3 bump_prerelease("") 1.0.0-1 Lower version!
1.0.0-alpha.3 bump_prerelease("rc") 1.0.0-rc
1.0.0-alpha.3 bump_prerelease("rc.0") 1.0.0-rc.0
1.0.0-alpha.3 bump_prerelease("rc.1") 1.0.0-rc.1

Either way, we will need to specify a new default token value. It can't be rc anymore. It must be either rc.0 or rc.1. Because if we introduce the handling of unnumbered alphanumerics, we must give the user the possibility to say that he wants to introduce an unnumbered alphanumeric identifier, i.e. by specifying token=<alphanumeric_identifier>.

I also think that the keyword argument token should be renamed to identifier (semver spec talks about identifiers, not tokens),

@fleetingbytes
Copy link
Contributor Author

fleetingbytes commented Nov 21, 2022

method name bump_prerelease is increasingly becoming a misnomer. It's more a change_prerelease method than a bump. For real bumping we have next_version

Old version API call New version Comment
1.0.0 next_version("prerelease") 1.0.1-rc.1 or rc.0
1.0.0 next_version("prerelease", identifier="") 1.0.1-1 or 0
1.0.0 next_version("prerelease", identifier="alpha") 1.0.1-alpha
1.0.0 next_version("prerelease", identifier="alpha.0") 1.0.1-alpha.0
1.0.0 next_version("prerelease", identifier="alpha.1") 1.0.1-alpha.1

etc.

@fleetingbytes
Copy link
Contributor Author

fleetingbytes commented Nov 21, 2022

We should also think about the variable type of the keyword token (or identifier as it could be called in the future). Since users would occasionally pass it values like alpha.1, it is actually not one identifier, but two, potentially even more. Hence it could be a tuple type or list type (more precisely, a Optional[Iterable[Union[Alphanumeric_Identifier, Numeric_Identifier]]]. The identifiers separator . is enforced by the semver spec anyway, so the user has no power overriding it. Consequently, he should not be obliged to write it when passing multiple identifiers. The API call should use identifiers (note the plural):

change_prerelease(identifiers=("alpha", 1))
next_version(part="prerelease", identifiers=("alpha", 1))

the new default value would then be the tuple ("rc", 1) – or ("rc", 0) rather than the string rc.1 or rc.0

For single identifiers we would have to use a one-tuple ("rc", ), (1, ), and for empty strings ("", ).

There still can be an additional method to parse a string of concatenated identifiers into an identifier iterable, but the underlying API should not be passed the identifier separator, ever .e.g:

v = Version.parse("1.0.0")
v.change_prerelease(identifiers=Version.parse_identifiers("alpha.1"))

If, hypothetically, Semver 3.0.0 would decide that the new separator is : rather than ., we shall not have to change the prerelease and build bumping methods or any other methods (e.g. _nat_comp, parse_identifiers). We should rather just change the read-only value of some Version.IDENTIFIER_SEPARATOR from . to : and all our methods would instantly work with the new separator.

@tomschr
Copy link
Member

tomschr commented Nov 23, 2022

Thanks for the overview. This is really helpful. 👍

I need to digest it a bit longer and will respond later. Many thanks.

@tomschr
Copy link
Member

tomschr commented Mar 5, 2023

@Nagidal Just for your information: I've merged PR #365 which fixes the .bump_prerelease() and .bump_build() methods. As you suggested, the default numeric identifiers starting at 1. This is implemented now.

Regarding the misnomer of token I'm still thinking about it. Your suggestion of using identifiers sounds interesting. For the time being, I'd like to keep this issue open. I would prefer to look again into this issue after semver 3 is released.

Thanks for all your ideas!

@fleetingbytes
Copy link
Contributor Author

@tomschr Thanks for the update.

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

No branches or pull requests

2 participants