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

Fix cz bump with custom type, scope and exclamation mark #774

Merged
merged 2 commits into from
Oct 14, 2023

Conversation

jmattheis
Copy link
Contributor

Description

This change adds support for version bumping for custom types with a scope. Previously, a commit with the following message didn't increment the version.

$ git commit --allow-empty -m 'chore(scope)!: major change'
[master 0567ca4] chore(scope)!: major change
$ python ./commitizen/cli.py bump --dry-run
bump: version 3.5.2 → 3.5.2
tag to create: v3.5.2

[NO_COMMITS_TO_BUMP]
The commits found are not eligible to be bumped

Also, there is a minor bugfix in the bump command, that resulted in a wrongly calculated increment. (It's separated into a commit)

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

Not sure if I have to update the documentation, because I'd say it's the expected behavior from the conventional commit specification.

Expected behavior

See steps to reproduce.

Steps to Test This Pull Request

  1. Add a commit
    git commit --allow-empty -m 'chore(scope)!: major change'
    
  2. Check that bump increments the major version
    $ python ./commitizen/cli.py bump --dry-run
    bump: version 3.5.2 → 4.0.0
    tag to create: v4.0.0
    increment detected: MAJOR
    

Additional context

Related to #673

@codecov
Copy link

codecov bot commented Jun 26, 2023

Codecov Report

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

Files Coverage Δ
commitizen/__version__.py 100.00% <100.00%> (ø)
commitizen/bump.py 100.00% <100.00%> (ø)
commitizen/cli.py 94.20% <ø> (ø)
commitizen/commands/bump.py 97.66% <100.00%> (+0.01%) ⬆️
commitizen/commands/commit.py 98.64% <100.00%> (+0.05%) ⬆️
commitizen/config/json_config.py 100.00% <100.00%> (+6.45%) ⬆️
commitizen/config/toml_config.py 100.00% <100.00%> (ø)
commitizen/config/yaml_config.py 100.00% <100.00%> (ø)
commitizen/defaults.py 98.38% <100.00%> (-1.62%) ⬇️
commitizen/providers/__init__.py 100.00% <100.00%> (ø)
... and 9 more

... and 1 file with indirect coverage changes

📢 Thoughts on this report? Let us know!.

@evanjmg
Copy link

evanjmg commented Sep 21, 2023

Please merge this.  

@woile
Copy link
Member

woile commented Oct 11, 2023

Looks good! Could you rebase please?
@Lee-W what do you think?

Copy link
Member

@Lee-W Lee-W left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mostly looks good to me. left one minor question

@jmattheis
Copy link
Contributor Author

@Lee-W what's the minor question?

@@ -104,7 +104,7 @@ class Settings(TypedDict, total=False):
MINOR = "MINOR"
PATCH = "PATCH"

bump_pattern = r"^(((BREAKING[\-\ ]CHANGE|feat|fix|refactor|perf)(\(.+\))?(!)?)|\w+!):"
bump_pattern = r"^((BREAKING[\-\ ]CHANGE|\w+)(\(.+\))?!?):"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we remove feat|fix|refactor|perf)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understood commitizen allows you to define custom commit types, if we'd limit this to only feat|fix|refactor|perf then it wouldn't match for custom types.

Previously \w+! was included, but this didn't include the scope, the new regex is basically a simplified version that allows \w+! with a scope.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? This change should be backwards compatible and it does fix a bug. chore! updates the major version but chore(scope)! does not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, I think you're right. looks good to me. thanks for helping out!

@Lee-W
Copy link
Member

Lee-W commented Oct 11, 2023

@Lee-W what's the minor question?

https://github.com/commitizen-tools/commitizen/pull/774/files#r1354592313

did not succeeded sending it earlier 🤦‍♂️

@Lee-W
Copy link
Member

Lee-W commented Oct 11, 2023

I think after we fix the CI and the last comment. We're good to go

The old implementation wrongly set the `increment` variable to `None` if
`increment = "PATCH"` and `new_increment = None`.
The regex didn't match the following commit with exclamation mark, and
therefore didn't update the version.

    chore(deps)!: drop support for Python 3.9

The regex was simplified, because a lot groups were not needed.
@Lee-W Lee-W requested review from woile and noirbizarre October 12, 2023 13:38
@Lee-W Lee-W added pr-status: ready-to-merge almost ready to merge. just keep it for a few days for others to check and removed pr-status: wait-for-review labels Oct 12, 2023
@Lee-W
Copy link
Member

Lee-W commented Oct 12, 2023

@woile @noirbizarre this pr looks good to me. plan to merge it these days. please let me know if you want to take a look

@Lee-W Lee-W merged commit 8f19b24 into commitizen-tools:master Oct 14, 2023
@jmattheis jmattheis deleted the bump_with_scope branch October 14, 2023 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-status: ready-to-merge almost ready to merge. just keep it for a few days for others to check
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants