-
-
Notifications
You must be signed in to change notification settings - Fork 301
feat(check): Add --allow-abort option
#512
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
8199666 to
cd8c895
Compare
Codecov Report
@@ Coverage Diff @@ ## master #512 +/- ## ======================================= Coverage 97.92% 97.92% ======================================= Files 39 39 Lines 1540 1543 +3 ======================================= + Hits 1508 1511 +3 Misses 32 32
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
LGTM but cz check is not my speciality, can u take a look @Lee-W ?
Explicitly mark some loop variables unused as recommended by flake8-bugbear.
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 @Kurt-von-Laven , sorry for late reviewing. Your work is fantastic! Just left a few questions. But I think we're almost ready to mrege
--allow-abort option (追記ここまで)
cd8c895 to
3c3e9ae
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.
Would it make sense for --allow-abort to also be added as an arg to the commit command?
EDIT: I think I answered this question for myself: no.
I am not able to reproduce the failure on Python 3.10 locally on Python 3.10.4. It appears to be unrelated to this pull request, so I think the build may simply need to be re-run. Please let me know if anyone sees a connection I am missing.
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.
Except for the minor comment update, this PR looks good to me. @woile I'm planning on merging it this weekend if no other new comments added
hmmm. we might need to take a deeper look at the 3.10 case
Empty commit messages indicate to Git that the commit should be aborted. Displaying an error message when the commit is already being aborted typically only creates confusion. Add an --allow-abort argument to the check command and allow_abort config variable, both defaulting to false for backwards compatibility. When the commit message is empty, determine the outcome based on the allow_abort setting alone, ignoring the pattern.
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 am still unable to reproduce the build failure locally on Python 3.10.4.
3c3e9ae to
aa25391
Compare
Looks like the Python 3.10 build indeed simply needed to be re-run since all I changed was the two comments.
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.
LGTM. Let's merge it now!
Uh oh!
There was an error while loading. Please reload this page.
Description
Empty commit messages indicate to Git that the commit should be aborted. Displaying an error message when the commit is already being aborted typically only creates confusion. Add an
--allow-abortargument to the check command andallow_abortconfig variable, both defaulting to false for backwards compatibility. When the commit message is empty, determine the outcome based on theallow_abortsetting alone, ignoring the pattern.Closes #424.
Checklist
./scripts/formatand./scripts/testlocally to ensure this change passes linter check and testExpected behavior
No error message is displayed when aborting a commit locally, unless the commit .
Steps to Test This Pull Request
cz commit.allow_abort = trueto your config, but an error occurs otherwise.Additional context
Would it make sense for
--allow-abortto also be added as an arg to thecommitcommand? How do folks feel about the argument/setting name and overall approach? See also #247 for a related feature proposal. Since the time of that discussion, I realized that naming a Commitizen argument--allow-empty-messagecould prove extremely confusing, because passing it to Commitizen would not, by itself, cause commits with empty messages to be committed like the Git argument of the same name. It would also interact poorly with the proposal to allow ferrying arbitrary arguments togit commitin #451, because it would be easy to accidentally pass the argument to Git, resulting in commits with empty messages no longer being aborted.