fix: properly bump versions between prereleases

[eduardocardoso]

Description

Properly bump version numbers between prereleases respecting the commit messages bump levels.

Checklist

• Add test cases to all the changes you introduce
• Run ./scripts/format and ./scripts/test locally to ensure this change passes linter check and test
• Test the changes on the local machine manually
• Update the documentation for the changes

Expected behavior

Bump command should bump the version number according to the commit messages since the last final version when bumping between prereleases.

Steps to Test This Pull Request

start at version 0.1.0

1. git commit --allow-empty --message 'fix: a fix'
2. cz bump --prerelease alpha → bumps to 0.1.1a0
3. git commit --allow-empty --message 'feat: a feature'
4. cz bump --prerelease alpha → bumps to 0.2.0a0 instead of 0.1.1a1

Additional context

#688

[codecov]

Codecov Report

Attention: 31 lines in your changes are missing coverage. Please review.

Comparison is base (120d514) 97.33% compared to head (7b4596a) 96.59%.
Report is 119 commits behind head on master.

Files	Patch %	Lines
commitizen/commands/bump.py	34.48%	19 Missing ⚠️
commitizen/cli.py	82.14%	5 Missing ⚠️
commitizen/providers/scm_provider.py	91.17%	3 Missing ⚠️
commitizen/git.py	80.00%	2 Missing ⚠️
commitizen/changelog_formats/__init__.py	97.77%	1 Missing ⚠️
commitizen/version_schemes.py	96.96%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## master #799 +/- ## ========================================== - Coverage 97.33% 96.59% -0.75%  ========================================== Files 42 55 +13 Lines 2104 2379 +275 ========================================== + Hits 2048 2298 +250 - Misses 56 81 +25 

Flag	Coverage Δ
unittests	96.59% <94.70%> (-0.75%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Lee-W]

This might not be the behavior we want. May I know why you want commitizen to behave this way?

[woile]

Could you update this list with more samples from this PR?

https://github.com/commitizen-tools/commitizen/blob/master/tests/test_version_scheme_pep440.py

@Lee-W the discussion in #688 talks about this change (if I understand correctly)

[jenstroeger]

@Lee-W the discussion in #688 talks about this change (if I understand correctly)

@Lee-W@woile yes that’s the issue where we talked about bumping pre-release versions.

[Lee-W]

Sounds good 👍

[Lee-W]

@eduardocardoso Could you please rebase from the main branch? I'll take a deeper look this or next week. Thanks!

[rgarrigue]

Hi. We'd be quite interested in this fix. Is it possible to push it forward if @eduardocardoso can't ?

[jenstroeger]

Hi. We'd be quite interested in this fix. Is it possible to push it forward if @eduardocardoso can't ?

@rgarrigue I talked with Eduardo and he’s busy; I can take a look at this next week?

[rgarrigue]

Would be cool, thanks !

[WarKaa]

+1 Is it scheduled for release soon?

[jenstroeger]

@WarKaa I’ll noodle on the PR tomorrow…

[jenstroeger]

@Lee-W This might not be the behavior we want. May I know why you want commitizen to behave this way?

Did you take a look at the discussion in issue #688, and are you ok with the approach?

@Lee-W Could you please rebase from the main branch?

I rebased the PR on today’s main branch (commit e9647c7).

@woile Could you update this list with more samples from this PR?

https://github.com/commitizen-tools/commitizen/blob/master/tests/test_version_scheme_pep440.py

You mean you’d like me to add a few more scenarios/examples to one of the lists of test cases?

Things left to do:

• Update the documentation for the changes: probably elaborate on the bump page
• Add a few more tests that test for alpha, beta, and/or rc pre-release suffixes

[woile]

You mean you’d like me to add a few more scenarios/examples to one of the lists of test cases?

Yes, to model all the new behavior on this pr 👍🏻

[jenstroeger]

@Lee-W@woile I noticed this test case:

commitizen/tests/test_version_scheme_pep440.py

Line 30 in af11332

(("1.0.0rc0", "PATCH", None, 0, None), "1.0.0"),

which I think isn’t right: if the current version is 1.0.0rc0 and I add a fix: commit and then bump the next release version (i.e. no pre-release) then the next version should be 1.0.1, no?

Similarly here:

commitizen/tests/test_version_scheme_pep440.py

Line 63 in af11332

(("1.0.0rc1+e20d7b57f3eb", "PATCH", None, 0, None), "1.0.0"),

where I expected 1.0.1 after the bump 🤔

Unfortunately, I didn’t find details on this problem over at the discussions for semver or conventional commits, so we can either raise that discussion or improvise. Intuitively, though, I think the above tests should be “fixed”?

[woile]

My understanding is that a release candidate can still receive patches and fixes until it's ready, even breaking changes. That's why you'd go from 1.0.0a0 to 1.0.0. There can be fixes between each "pre-release".

For example, the popular bootstrap does something like this. see releases

Same for axum (a popular rust web framework), see releases

[jenstroeger]

For example, the popular bootstrap does something like this. see releases […] Same for axum (a popular rust web framework), see releases

Neither of them uses conventional commits and actually computes the next release string from a given tag and commit range. Both kinda make things up manually.

My understanding is that a release candidate can still receive patches and fixes until it's ready, even breaking changes. That's why you'd go from 1.0.0a0 to 1.0.0. There can be fixes between each "pre-release".

Let’s look at this from another angle: we have a final release version 1.0.0 and then push a fix: commit followed by a feat: commit. That means that given the release version and two commits, the next release version would be 1.1.0 — right?

Now suppose we want to publish a release candidate with every commit. Thus, we should see the following: 1.0.0 bumps to 1.0.1rc0 after the fix: commit, then bumps to 1.1.0rc1 (or rc0 depending on convention?) after the feat: commit, then bumps to final release 1.1.0.

So with every release candidate we need to take into account the history since the last final release (which is what this PR actually does here).

Which also means that the above test

commitizen/tests/test_version_scheme_pep440.py

Line 30 in af11332

(("1.0.0rc0", "PATCH", None, 0, None), "1.0.0"),

is somewhat meaningless because we don’t know the history since the last final release version.

[woile]

Neither of them uses conventional commits

Both use semver

is somewhat meaningless because we don’t know the history since the last final release version

If you are in 1.0.0rc0 you know that either the API is being stabilized or there have been breaking changes. Any new patch or fix will be accumulated to the pre-release or will finally be released as 1.0.0.

Help me understand, and let’s look at this from another angle again. As a user, I want to start releasing candidates for my next major version: 2.0.0.
How would it look if we release a candidate coming from 1.9 for every following commit:

• breaking
• fix
• feat
• breaking
• fix
• feat

Could you list the versions until we make stable 2.0?

[woile]

btw this is docusaurus using semver and cc
https://github.com/facebook/docusaurus/releases

[noirbizarre]

Note:
I think can already customize yourself this behavior by doing your own version scheme and overriding the bump method.

Cf.

• https://github.com/commitizen-tools/commitizen/blob/master/commitizen/version_schemes.py#L96-L106
• https://commitizen-tools.github.io/commitizen/bump/#version_scheme

It doesn't mean that is use case doesn't need to be discussed, but that you can already customize it (while we need to agree on how this PR and behavior should be handled).

It initially wasn't meant to, but this is the beauty of making things extensible: you don't know what will come from the community with those extensions. Here's a perfect illustration.

[noirbizarre]

No, I used some process to illustrate why the discussed behavior would make sense. The discussed behavior is how cz bump --prerelease beta (for example) is expected to behave in the face of a previous final release and one or more previous pre-releases.

Oh, I understood, and I tried to show you how it is your expectations' by showing you counter examples (the fact that PEP440 on which is based this soft is saying otherwise) and the fact that all 3 reviewers are reluctant to merge the PR in the current state might indicate that your expectations are not shared.
Sorry about that, not my place to tell and after review, not what I imagined. 🙏🏼

You might want to look at @woile’s comment #800 (comment) where he suggests using another command-line switch to refine the behavior of cz bump under a --prerelease switch.

Again, that’s what we’re discussing here.

I did and I even linked the answer in my previous response, there 👇🏼

(seems like the same idea as #800 (comment) but with the opposite flag and nominal case)

As I said, I have no issue with your workflow, I have an issue with making it the nominal behavior. Same as I would have an issue with making the "prevent feature commit on pre-release" the nominal behavior. Both are something which are tied to teams' usage and expectations.
I strongly think these kinds of changes should be opt-in or extensions based, not the normal behavior.

[noirbizarre]

After review, I am OK with it (except for a few nitpicky naming suggestions 👇🏼), tests are passing, no changes for existings 👍🏼

[noirbizarre]

To keep the same naming convention (and try to contain semver wording to the version scheme implementation)

Suggested change

	current_semver=".".join(str(part) forpartinrelease)
	ifbase==current_semver:
	current_base=".".join(str(part) forpartinrelease)
	ifbase==current_base:

[jenstroeger]

Commit 9da9302.

[noirbizarre]

Good API addition 👍🏼

[jenstroeger]

We should probably rename the function to find_base_version() because “base version” seems to be the term used in other places.

[noirbizarre]

To keep the same naming convention (and try to contain semver wording to the version scheme implementation)

Suggested change

	semver=last_final.increment_base(
	increment=increment, force_bump=True
	)
	ifsemver!=current_version.base_version:
	base=last_final.increment_base(
	increment=increment, force_bump=True
	)
	ifbase!=current_version.base_version:

[jenstroeger]

Commit 9da9302.

[woile]

Hey @jenstroeger don't take it wrong, but I still have trouble understanding your comment on this particular case, why should it be removed? what is wrong with it?

commitizen/tests/test_version_scheme_pep440.py

Line 30 in af11332

(("1.0.0rc0", "PATCH", None, 0, None), "1.0.0"),

I follow the logic of your other examples, but they are always for non-breaking changes. And you said this test case is irrelevant. But this test case, would be for a breaking change, how would the logic be?

If my understanding is correct, from any given version X, you can go to 3 possible pre-release versions (X + patch + pr, X + minor + pr, X + break + pr), and any new pre-release won't go beyond one of those 3 pre-release versions (depending on the given semver increment given). The final version will be one of those possibilities without the pre-release. Now if the example is 1.0.0rc0 why wouldn't the next version be 1.0.0?

This is how I think about the problem

graph LR; a[1.9.0]-->d[major: 2.0.0rc0] a[1.9.0]-->c[minor: 1.10.0rc0] a[1.9.0]-->b[patch: 1.9.1rc0] d --> e[major: 2.0.0rc1] c --> e c --> f[minor: 1.10.0rc1] b --> e b --> g[patch: 1.9.1rc1] b --> f e --> f1[2.0.0] f --> f1 f --> f2[1.10.0] g --> f1 g --> f2 g --> f3[1.9.1] 

Loading

And from 2.0.0rc0 I see only one option: 2.0.0. According to your comment, you propose going to 2.0.1

Apologies again for my struggle 😅 🙏🏻

[jenstroeger]

Hi @woile — no worries, it’s great we have these discussions to clarify how cz envisions to bump. Unfortunately, none of the bumping behavior is specified by SemVer or Conventional Commits. (We should probably spark a discussion there?)

What I’m trying to say is that given a pre-release tag and a commit, we can’t actually compute a next pre-release or final release tag because in both cases we need the history since the last final release. In your graph above, for every node that’s two or more commits away from the last final release (in the above case 1.9.0) it is impossible to bump without considering the history since the last final release. After some staring and thinking and scratching my head, I think you’re right.

Looking at your graph, I think I see what you’re saying: if my current pre-release tag is

• 2.0.0rc1 then both minor and patch are 0 and therefore the accumulated commit history must contain at least one breaking change, any other minor and patch commit are included already and we just keep incrementing the rcX counter.
• 1.10.0.rc1 then only the patch version is 0 and therefore the accumulated commit history must contain at least one minor bump and no breaking change, and other minor and patch commit are included already and we just keep incrementing the rcX, unless the commit is a breaking change in which case we’d go to 2.0.0rc2 (or final release 2.0.0).
• 1.9.1rc1 then only the patch version has been bumped and therefore the accumulated commit history must contain only patch bumps and no minor or breaking change, and the next pre-release bump can bump anything depending on the commit.

That makes sense 👍🏼

With that in mind, the test case

commitizen/tests/test_version_scheme_pep440.py

Line 30 in af11332

(("1.0.0rc0", "PATCH", None, 0, None), "1.0.0"),

tells us that between the last final release and this pre-release there was one commit (because the rc0 counter is 0) which was a breaking change (because we’re now at 1.0.0rc0 and both minor and patch are 0) and thus a patch is already covered and we either bump to

• 1.0.0rc1 for the next pre-release, or
• 1.0.0 for the next final release.

Also, I think it makes a lot of sense to keep the pre-release increment counting up every time, for example if I’m at 1.10.0rc2 and I commit a test: or chore: then the next pre-release is 1.10.0rc3.

[woile]

Okay we are aligned then 👍🏻 It's good to go

Does it also close #757 and #751 ?

test: or chore: then the next pre-release is 1.10.0rc3

probably yes, but wondering if it should be related to the issue discussing the use of test:, etc. (cannot find the link)

[jenstroeger]

@woile I have a change to the documentation pending, and a few more tests. Let me add tests to check the two other issues and then we’re good to merge after.

But also keep in mind that with this change we’ll have to merge #800 — or we can pull #800 into this PR?

[jenstroeger]

I added a bunch of tests and had to refactor some code: BaseVersion.bump() didn’t handle all pre-release scenarios.

There’s one more open issue with the following tests:

(("3.1.4", None, "alpha", 0, None), "3.1.4a0"), # No pump, just adding the pre-release suffix. (("3.1.4a0", "PATCH", "alpha", 0, None), "3.1.4a1"), # UNEXPECTED! (("3.1.4a0", "MINOR", "alpha", 0, None), "3.2.0a0"), (("3.1.4a0", "MAJOR", "alpha", 0, None), "4.0.0a0"),

These tests pass.

So here we have a final version 3.1.4 and without any proper release commit (e.g. a chore:) we “bump” to pre-release version 3.1.4a0. If the next commit is a fix: then we bump to 3.1.4a1 but should bump to 3.1.5a0 🤔

I can push my changes for discussion, if it helps?

[jenstroeger]

@woile@noirbizarre sorry for the delay: I didn’t receive an email notification for the 👍🏼 on the message. Anyway, I pushed my local updates which contain:

• A refactor of the bump code: lowered bumping into the BaseVersion.bump() method and cleaned up the callers of that function. That came out of existing tests that call Pep440.bump() instead of going through the CLI bump command line (which are separate tests I’ve not yet expanded).
• More tests that play through various scenarios that bump pre-releases, including the unexpected behavior I mentioned above.

Please do take a close look at the code and the current behavior, and let me know what you think. At this point, I believe that pre-release bumping must take into account the last base version and not just the current tag.

[woile]

Regarding #800 I think you can pull it in this one as well

[jenstroeger]

Regarding #800 I think you can pull it in this one as well

Yes I agree.

I need to check, but also we should make sure that final published versions contain the full changelog since the last final published version — not just the changelog since the last tag (which can be any one of the pre-releases).

But we still need to discuss this particular case which misses a patch bump:

(("3.1.4", None, "alpha", 0, None), "3.1.4a0"), # No pump, just adding the pre-release suffix. (("3.1.4a0", "PATCH", "alpha", 0, None), "3.1.4a1"), # UNEXPECTED!

[noirbizarre]

Good to me, agreed on base version precedence over the tag (can't be otherwise if we want to be able to collapse pre-release on the stable release).
I also agree for #800 inclusion.

For the (("3.1.4a0", "PATCH", "alpha", 0, None), "3.1.4a1") unexpected case, I'll tend to say that if we merge this PR, it needs to be consistent all the way and so result in:

(("3.1.4a0", "PATCH", "alpha", 0, None), "3.1.5a0"),

But I don't have a strong opinion on this because to me this call is an edge case. I consider IRL this case needs to be safeguarded by a process decided projects maintainers: either you are in a stabilization phase where you increment a prerelease number, either you have published the stable version and you are doing a patch.

[jenstroeger]

I rebased the PR, merged the changes from #800, and updated the documentation.

To address the following unexpected behavior:

(("3.1.4", None, "alpha", 0, None), "3.1.4a0"), # No pump, just adding the pre-release suffix. (("3.1.4a0", "PATCH", "alpha", 0, None), "3.1.4a1"), # UNEXPECTED!

I think we can’t uphold the assumption that a bump is always context free. Instead, a bump needs to take into account the previous history back to the most recent final release. (@woile that changes our previous conversation in this comment.)

@woile Does it also close #757 and #751 ?

I’ll have to check.

[woile]

I'll take a last look after holidays and merge

[jenstroeger]

@woile I'll take a last look after holidays and merge

No worries. Only point for discussion would be the unexpected behavior mentioned above.

[chadrik]

This looks great. Would love to see this merged soon!