Skip to content

feat: check if commit message is no more than 72 characters #557

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

kevin1kevin1k
Copy link
Contributor

@kevin1kevin1k kevin1kevin1k commented Aug 14, 2022

Description

Check if the length of commit message (the prefix(scope): subject part) does not exceed 72 characters.
Currently only supports ConventionalCommitsCz; JiraSmartCz and CustomizeCommitsCz not implemented.
An imperfect workaround for #191.

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

Expected behavior

If run cz commit with -cl or --check-length argument, the commit would fail when prefix(scope): subject is longer than 72 chars.
Without the argument, the default behavior is to accept longer messages.

Steps to Test This Pull Request

  1. cz commit -cl
  2. ENTER (type prefix: fix)
  3. ENTER (empty scope)
  4. 01234567890123456789012345678901234567890123456789012345678901234567 (for subject)
  5. ENTER (empty body)
  6. ENTER (not breaking change)
  7. ENTER (no footer)

Then cz should exit with message Length of commit message exceeded limit (73/72) and error code 23.

Additional context

@codecov
Copy link

codecov bot commented Aug 14, 2022

Codecov Report

Merging #557 (44c145d) into master (764861f) will decrease coverage by 0.00%.
The diff coverage is 98.63%.

@@            Coverage Diff             @@
##           master     #557      +/-   ##
==========================================
- Coverage   98.25%   98.25%   -0.01%     
==========================================
  Files          39       39              
  Lines        1551     1604      +53     
==========================================
+ Hits         1524     1576      +52     
- Misses         27       28       +1     
Flag Coverage Δ
unittests 98.25% <98.63%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
commitizen/cli.py 93.93% <ø> (ø)
...en/cz/conventional_commits/conventional_commits.py 98.59% <88.88%> (-1.41%) ⬇️
commitizen/__version__.py 100.00% <100.00%> (ø)
commitizen/bump.py 100.00% <100.00%> (ø)
commitizen/cmd.py 100.00% <100.00%> (ø)
commitizen/commands/changelog.py 96.62% <100.00%> (+0.03%) ⬆️
commitizen/commands/commit.py 98.50% <100.00%> (+0.09%) ⬆️
commitizen/commands/init.py 91.66% <100.00%> (ø)
commitizen/config/json_config.py 100.00% <100.00%> (ø)
commitizen/config/yaml_config.py 100.00% <100.00%> (ø)
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Lee-W Lee-W self-requested a review August 16, 2022 09:54
@Lee-W Lee-W self-assigned this Aug 16, 2022
@@ -29,6 +29,8 @@ def __init__(self, config: BaseConfig, arguments: dict):
self.config: BaseConfig = config
self.cz = factory.commiter_factory(self.config)
self.arguments = arguments
print(self.config)
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest we don't print anything not directly related to cz commit

message = f"{prefix}{scope}: {subject}{body}{footer}"
message = f"{prefix}{scope}: {subject}"
message_len = len(message)
MESSAGE_LEN_LIMIT = 72
Copy link
Member

Choose a reason for hiding this comment

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

Could we make it configurable?

Copy link
Member

Choose a reason for hiding this comment

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

Also, in the comment #191 (comment), he proposes a reasonable solution. Although global variables might sometimes not be desired. I'm good with it in this case. @kevin1kevin1k @woile What do you think?

@@ -44,7 +45,7 @@ def questions(self) -> Questions:
]
return questions

def message(self, answers) -> str:
def message(self, answers: dict, check_length: Optional[bool] = False) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could implement such check on customize and jira as well 🤔

@woile
Copy link
Member

woile commented Apr 28, 2023

What's the status of this PR? Do we still want this feature?

@Lee-W
Copy link
Member

Lee-W commented May 6, 2023

I think this feature is still what we want but do not have bandwidth to work on it at this moment

@schlotter
Copy link
Contributor

This PR is useful. What is missing to get it merged?

@Lee-W
Copy link
Member

Lee-W commented Jul 15, 2023

This PR is useful. What is missing to get it merged?

I think we should make it a configurable feature.

@schlotter
Copy link
Contributor

schlotter commented Jul 16, 2023

@kevin1kevin1k Could you please make MESSAGE_LEN_LIMIT configurable? I think this would be a much wanted improvement!

@kevin1kevin1k
Copy link
Contributor Author

Sorry for the late response.
I will find time to resolve the comments and make it configurable!

@nowNick
Copy link

nowNick commented Nov 3, 2023

Hey :)
What's the status of this PR? @kevin1kevin1k - do you think you'll have some time to wrap it up 🙇 ?

@kevin1kevin1k
Copy link
Contributor Author

Thanks for the reminder!
I've still been busy lately, but I guess I can find time to finish it up in the upcoming weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants