fix: properly bump versions between prereleases #799
Conversation
Codecov ReportAttention:
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
This might not be the behavior we want. May I know why you want commitizen to behave this way? |
|
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) |
|
Sounds good 👍 |
|
@eduardocardoso Could you please rebase from the main branch? I'll take a deeper look this or next week. Thanks! |
Lee-W
added
pr-status: wait-for-review
pr-status: wait-for-modification
and removed
pr-status: wait-for-response
labels
|
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? |
|
Would be cool, thanks ! |
|
+1 Is it scheduled for release soon? |
|
@WarKaa I’ll noodle on the PR tomorrow… |
jens-getpassport
force-pushed
the
prerelease-bumps
branch
from
c68a231 to
8522b8c
Compare
Did you take a look at the discussion in issue #688, and are you ok with the approach?
I rebased the PR on today’s main branch (commit e9647c7).
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:
|
Yes, to model all the new behavior on this pr 👍🏻 |
|
@Lee-W @woile I noticed this test case: which I think isn’t right: if the current version is Similarly here: where I expected 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”? |
|
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 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.
Let’s look at this from another angle: we have a final release version Now suppose we want to publish a release candidate with every commit. Thus, we should see the following: 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 is somewhat meaningless because we don’t know the history since the last final release version. |
Both use semver
If you are in 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:
Could you list the versions until we make stable |
btw this is docusaurus using semver and cc |
|
Note: Cf.
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. |
I did and I even linked the answer in my previous response, there 👇🏼
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. |
commitizen/version_schemes.py
Outdated
| current_semver = ".".join(str(part) for part in release) | ||
| if base == current_semver: |
There was a problem hiding this comment.
To keep the same naming convention (and try to contain semver wording to the version scheme implementation)
| current_semver = ".".join(str(part) for part in release) | |
| if base == current_semver: | |
| current_base = ".".join(str(part) for part in release) | |
| if base == current_base: |
| @@ -395,3 +411,33 @@ def _get_commit_args(self): | |||
| if self.no_verify: | |||
| commit_args.append("--no-verify") | |||
| return " ".join(commit_args) | |||
|
|
|||
| def find_previous_final_version( | |||
There was a problem hiding this comment.
We should probably rename the function to find_base_version() because “base version” seems to be the term used in other places.
commitizen/commands/bump.py
Outdated
| semver = last_final.increment_base( | ||
| increment=increment, force_bump=True | ||
| ) | ||
| if semver != current_version.base_version: |
There was a problem hiding this comment.
To keep the same naming convention (and try to contain semver wording to the version scheme implementation)
| semver = last_final.increment_base( | |
| increment=increment, force_bump=True | |
| ) | |
| if semver != current_version.base_version: | |
| base = last_final.increment_base( | |
| increment=increment, force_bump=True | |
| ) | |
| if base != current_version.base_version: |
|
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? 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 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]
And from Apologies again for my struggle 😅 🙏🏻 |
|
Hi @woile — no worries, it’s great we have these discussions to clarify how
Looking at your graph, I think I see what you’re saying: if my current pre-release tag is
That makes sense 👍🏼 With that in mind, the test case tells us that between the last final release and this pre-release there was one commit (because the
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 |
|
I added a bunch of tests and had to refactor some code: 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 I can push my changes for discussion, if it helps? |
jens-getpassport
force-pushed
the
prerelease-bumps
branch
from
9da9302 to
b16bcbe
Compare
|
@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:
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. |
|
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! |
|
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). For the (("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. |
jens-getpassport
force-pushed
the
prerelease-bumps
branch
from
b16bcbe to
7b4596a
Compare
|
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.) I’ll have to check. |
|
I'll take a last look after holidays and merge |
No worries. Only point for discussion would be the unexpected behavior mentioned above. |
|
This looks great. Would love to see this merged soon! |
Description
Properly bump version numbers between prereleases respecting the commit messages bump levels.
Checklist
./scripts/formatand./scripts/testlocally to ensure this change passes linter check and testExpected 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
0.1.1a00.2.0a0instead of0.1.1a1Additional context
#688