-
-
Notifications
You must be signed in to change notification settings - Fork 298
fix: properly bump versions between prereleases #799
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
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!
rgarrigue
commented
Oct 24, 2023
Hi. We'd be quite interested in this fix. Is it possible to push it forward if @eduardocardoso can't ?
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
commented
Oct 27, 2023
Would be cool, thanks !
WarKaa
commented
Oct 31, 2023
+1 Is it scheduled for release soon?
@WarKaa I’ll noodle on the PR tomorrow...
c68a231
to
8522b8c
Compare
@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/orrc
pre-release suffixes
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 👍🏻
@Lee-W @woile I noticed this test case:
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:
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"?
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
Screenshot 2023年11月06日 at 08 39 37Same for axum (a popular rust web framework), see releases
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
to1.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
is somewhat meaningless because we don’t know the history since the last final release version.
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
?
btw this is docusaurus using semver and cc
https://github.com/facebook/docusaurus/releases
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.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After review, I am OK with it (except for a few nitpicky naming suggestions 👇🏼), tests are passing, no changes for existings 👍🏼
commitizen/version_schemes.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To keep the same naming convention (and try to contain semver wording to the version scheme implementation)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit 9da9302.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good API addition 👍🏼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To keep the same naming convention (and try to contain semver wording to the version scheme implementation)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit 9da9302.
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 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]
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 😅 🙏🏻
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 After some staring and thinking and scratching my head, I think you’re right.1.9.0
) it is impossible to bump without considering the history since the last final release. (削除ここまで)
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 are0
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 thercX
counter.1.10.0.rc1
then only the patch version is0
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 thercX
, unless the commit is a breaking change in which case we’d go to2.0.0rc2
(or final release2.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
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, or1.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
.
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?
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:
- 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 callPep440.bump()
instead of going through the CLIbump
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.
Regarding #800 I think you can pull it in this one as well
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).
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.
...ers of BaseVersion.bump()
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
@woile 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/format
and./scripts/test
locally 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.1a0
0.2.0a0
instead of0.1.1a1
Additional context
#688