-
-
Notifications
You must be signed in to change notification settings - Fork 265
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
feat(ConventionalCommitsCz): allow to override defaults from config #546
base: master
Are you sure you want to change the base?
feat(ConventionalCommitsCz): allow to override defaults from config #546
Conversation
a5ed888
to
ed57fab
Compare
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.
Thanks for your contributions!
I'm not sure whether we're to deprecate CustomizeCommitsCz now. We'll do it eventually. But I think it might be better for us to keep it but add a deprecation warning (e.g., we're going to deprecate it on 3.0) @woile What do you think?
2b903f9
to
440a7fa
Compare
440a7fa
to
98c6fa1
Compare
As addressed on commitizen-tools#535, when using customize commitizen, if we want to customize a small attribute, we need to redefine all commitizens options to make our custom class work. For example: ```diff diff --git a/cz.yaml b/cz.yaml index f2e19a9..302e961 100644 --- a/cz.yaml +++ b/cz.yaml @@ -1,6 +1,18 @@ commitizen: annotated_tag: true bump_message: 'bump: $current_version -> $new_version [skip ci]' - name: cz_conventional_commits + name: cz_customize update_changelog_on_bump: true version: 0.11.0 + + customize: + bump_pattern: '^(fix|feat|docs|style|refactor|test|build|ci)' + bump_map: + fix: PATCH + feat: PATCH + docs: PATCH + style: PATCH + refactor: PATCH + test: PATCH + build: PATCH + ci: PATCH diff --git a/t b/t new file mode 100644 index 0000000..e69de29 diff --git a/t2 b/t2 new file mode 100644 index 0000000..e69de29 ``` making the following change on a repo would cause an unexpected behavior: ```python + bash -c cz commit Traceback (most recent call last): File "/home/amit/.local/bin/cz", line 8, in <module> sys.exit(main()) File "/home/amit/.local/lib/python3.10/site-packages/commitizen/cli.py", line 382, in main args.func(conf, vars(args))() File "/home/amit/.local/lib/python3.10/site-packages/commitizen/commands/commit.py", line 74, in __call__ m = self.prompt_commit_questions() File "/home/amit/.local/lib/python3.10/site-packages/commitizen/commands/commit.py", line 49, in prompt_commit_questions for question in filter(lambda q: q["type"] == "list", questions): File "/home/amit/.local/lib/python3.10/site-packages/commitizen/commands/commit.py", line 49, in <lambda> for question in filter(lambda q: q["type"] == "list", questions): KeyError: 'type' ``` From my best understanding, this error happens because I didn't defined question section in config, though I'm ok with using ConventionalCommitsCz default ones. This commit extends ConventionalCommitsCz to read from config and fallbacks to defaults if some are not provided. By adding this change, potentially customize commitizen can be deprecated. Closes commitizen-tools#535.
98c6fa1
to
6a993f7
Compare
@Lee-W Hi, made some changes and fixed tests, Can you take another look? |
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.
Hi @amitlevy21 , sorry for the super late reply. I just took a look and left some comments. Also, we might need your help to rebase from the latest master branch. Let me know if you have any though on it. Thanks!
if not self.config.settings.get("style"): | ||
self.config.settings.update({"style": BaseCommitizen.default_style_config}) | ||
self.style = Style(self.config.settings.get("style", defaults.style)) | ||
self.bump_pattern: Optional[str] = self.config.settings.get( |
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 say the defaults.bump_pattern
is something only for conventional commit
that I want to move out from default and move it to conventional commit. Thus, I'd like to keep the default value of this as None
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.
This behavior should be applied to all the following default values which did not exist in BaseCommitizen
from commitizen.cz.base import BaseCommitizen | ||
from commitizen.defaults import Questions | ||
|
||
__all__ = ["JiraSmartCz"] | ||
|
||
|
||
class JiraSmartCz(BaseCommitizen): | ||
def __init__(self, config: BaseConfig): | ||
super().__init__(config) | ||
self.bump_map = None |
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 think the values here is the ideal default values in BaseCommitizen
and we should move the defaults
to ConventionalCommitsCz
Before fixing tests and adding extra ones, I would like to receive
partial feedback about my changes to see if this aligns with the desired
code changes by maintainers.
As addressed on #535, when using customize commitizen, if we want to
customize a small attribute, we need to redefine all commitizens options
to make our custom class work.
For example:
making the following change on a repo would cause an unexpected
behavior:
From my best understanding, this error happens because I didn't defined
question section in config, though I'm ok with using
ConventionalCommitsCz default ones.
This commit extends ConventionalCommitsCz to read from config and
fallbacks to defaults if some are not provided.
By adding this change, potentially customize commitizen can be
deprecated.
A warning message will be printed when using
cz_customize
Closes #535.
Description
Checklist
./scripts/format
and./scripts/test
locally to ensure this change passes linter check and testExpected behavior
Steps to Test This Pull Request
Additional context