-
-
Notifications
You must be signed in to change notification settings - Fork 309
feat(version): add MANUAL_VERSION, --next and --patch to version command #1724
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
base: master
Are you sure you want to change the base?
feat(version): add MANUAL_VERSION, --next and --patch to version command #1724
Conversation
|
@Lee-W I already ran Do you have any ideas why this happen |
| version = f"{version_scheme.minor}" | ||
| out.write(version.major) | ||
| return | ||
| if self.arguments.get("minor"): |
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.
I realize now that this creates problems with the (not)monotonic kind of versions (and possible non-semver). I'm not sure what to do about it.
I think for now it's fine that if you diverge too much from semver in your custom version scheme, then you won't get the full range of features.
|
There was a way to run all the pre-commits, but I don't remember how it works 😅 |
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "next_version, current_version, expected_version", |
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.
maybe rename next_version to next_increment?
In the user facing cli, it makes sense semantically: --next=MAJOR (give me the next major).
| assert expected_version in captured.out | ||
|
|
||
|
|
||
| def test_next_version_invalid_version(config, capsys): |
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.
what about having a parametrize of the combinations that should fail:
@pytest.mark.parametrize(
"args",
[
# incomplete list:
{"next"},
# ...
{"--verbose", "next"}
]
)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.
Sounds good.
commitizen/commands/version.py
Outdated
| out.error("Invalid version.") | ||
| return | ||
|
|
||
| if next_str := self.arguments.get("next"): |
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.
what would happen here if I run cz version --project --next? I would expect that to work
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.
That also works.
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.
The behavior is the same as cz version --next=*
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.
But what would be the output?
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.
Oh I misunderstood your question. The output should depend on the commit history, right?
I forgot to implement that.
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.
I will do #1678 in another PR since it's a bit complicated (we need to consider major_version_zero, bump_map, etc. for this feature and I prefer not to just copy & paste code, need some refactoring)
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.
Indeed, that logic should be core and it should be possible to re-use it across commands.
|
Nice to see this going forward 🎉 |
54159ff to
34fb773
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1724 +/- ##
==========================================
- Coverage 97.88% 97.81% -0.08%
==========================================
Files 60 61 +1
Lines 2606 2649 +43
==========================================
+ Hits 2551 2591 +40
- Misses 55 58 +3 ☔ View full report in Codecov by Sentry. |
|
I will rebase this branch after #1726 is merged. The test failure should be resolved then. |
8d938d2 to
f9afbd4
Compare
f9afbd4 to
094f48b
Compare
3b48a33 to
b07121e
Compare
noirbizarre
left a comment
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.
That's an interesting feature, one that I would totally use!
Also, I never realized that the version command was doing totally different things depending on the flags 😅
So maybe it's the occasion to have a dedicated --version flag and have the version command focusing on the the project versioning and not the Commitizen version. It would be cleaner, easier to understand and more standard. WDYT ?
Note
I would not remove the Type aliases as it makes the PR bigger than it should and it introduce a breaking change (and reading the PR and issues, we know that it has been used by some users for some custom version schemes).
commitizen/version_increment.py
Outdated
| return self.name | ||
|
|
||
| @classmethod | ||
| def safe_cast(cls, value: object) -> "VersionIncrement": |
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.
Add from __future__ import annotations and remove the quotes to stay consistent with the existing code base.
Also it's not a cast but an instantiation, so maybe rename this in from_value()
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.
iirc there is a ruff rule for this. will check later when I have a moment
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.
I would prefer to merge this before #1518 because it is a bit larger than this PR.
+1. As a user, I often run |
b07121e to
044416d
Compare
575468d to
3af1a57
Compare
3af1a57 to
4b6a441
Compare
4b6a441 to
c3d27c9
Compare
Closes #1679
Manually tested and added test cases, the result LGTM.