fix: add support for commit message file argument for pre-commit#366
fix: add support for commit message file argument for pre-commit#366shenxianpeng merged 6 commits intomainfrom
Conversation
✅ Deploy Preview for commit-check ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughAdds "refactor" to the allowed commit imperatives and changes CLI parsing to accept a positional commit message file path. The Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI as commit-check main
participant File as commit_msg_file
participant Stdin
participant Git
participant Validator
User->>CLI: invoke (pre-commit / git commit)
CLI->>File: check positional `commit_msg_file` argument
alt file provided
File-->>CLI: commit_file_path
else no file
CLI->>Stdin: read stdin for message
alt stdin has content
Stdin-->>CLI: message content
else no stdin
CLI->>Git: request commit data
Git-->>CLI: commit content
end
end
CLI->>Validator: run validators with commit content (message validation enabled)
Validator-->>CLI: results
CLI-->>User: exit/status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #366 +/- ##
==========================================
+ Coverage 94.37% 94.65% +0.28%
==========================================
Files 9 9
Lines 800 805 +5
==========================================
+ Hits 755 762 +7
+ Misses 45 43 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging this PR will degrade performance by 31.33%Comparing Summary
Performance Changes
Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@commit_check/main.py`:
- Around line 319-327: The positional commit message file handling currently
sets args.message = True only when no other checks are present, causing message
validation to be skipped if a file is provided alongside --branch or --author
flags; change the logic in the commit_msg_file handling block so that when
args.commit_msg_file (commit_msg_file_path) is provided it always enables
message validation by setting args.message = True (or alternatively raise an
explicit error if combining a positional commit_msg_file with other checks is
unsupported); update the block that references commit_msg_file_path,
args.commit_msg_file, args.message, args.branch, args.author_name, and
args.author_email accordingly.
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #363 by adding support for pre-commit hooks to pass the commit message file as a positional argument. The pre-commit framework passes filenames as positional arguments when pass_filenames: true is set, causing the original implementation to fail with "unrecognized arguments" error.
Changes:
- Modified argument parsing to accept an optional positional
commit_msg_fileargument for pre-commit compatibility - Changed
--messageflag from accepting an optional file path to a boolean flag - Added "refactor" to the list of imperative verbs (unrelated to the main fix)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| commit_check/main.py | Added positional commit_msg_file argument and modified --message to be a boolean flag; updated logic to handle commit message file from either positional argument or stdin |
| commit_check/imperatives.py | Added "refactor" as an imperative verb (allows "refactor:" prefix in conventional commits) |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@shenxianpeng I've opened a new pull request, #367, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@commit_check/main.py`:
- Around line 65-66: Document that the semantics of the -m/--message flag
changed from accepting an optional file path to a boolean store_true flag:
update release notes and the migration guide to show the new usage (use the
positional argument for a message file, e.g., commit-check -m /path/to/file) and
add a clear note in the CLI help text for the --message/-m option in main.py so
users know to move prior invocations like `-m /path/to/file` to the positional
parameter.
| action="store_true", | ||
| help="validate commit message (file path can be provided as positional argument for pre-commit compatibility)", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's understand the current structure of the repository
git ls-files | head -20Repository: commit-check/commit-check
Length of output: 561
🏁 Script executed:
# Check the git history of main.py to see if --message behavior changed
git log --oneline commit_check/main.py | head -10Repository: commit-check/commit-check
Length of output: 120
🏁 Script executed:
# Look at the current main.py to understand the full context of lines 65-66 and the positional argument handling
wc -l commit_check/main.pyRepository: commit-check/commit-check
Length of output: 93
🏁 Script executed:
# Read the relevant section of main.py around lines 65-66 and surrounding code
sed -n '50,100p' commit_check/main.pyRepository: commit-check/commit-check
Length of output: 1332
🏁 Script executed:
# Check the recent commit that modified main.py
git show e593590 --statRepository: commit-check/commit-check
Length of output: 3037
🏁 Script executed:
# View the actual changes in that commit for main.py
git show e593590 -- commit_check/main.py | head -150Repository: commit-check/commit-check
Length of output: 4457
🏁 Script executed:
# Check if there's an even earlier version to understand the full history
git log -p --follow -S "--message" -- commit_check/main.py | head -200Repository: commit-check/commit-check
Length of output: 51
🏁 Script executed:
# Double-check if there are any earlier commits or branches with main.py
git log --all --follow --source -- commit_check/main.pyRepository: commit-check/commit-check
Length of output: 14151
🏁 Script executed:
# Verify the tests to see what the intended behavior is
grep -n "\-\-message\|commit_msg_file" tests/main_test.py | head -30Repository: commit-check/commit-check
Length of output: 1331
🏁 Script executed:
# Check the previous version of main.py from the commit before e593590
git show bd1abc3:commit_check/main.py | head -100Repository: commit-check/commit-check
Length of output: 2914
🏁 Script executed:
# Check if the old version had --message accepting a file path
git show bd1abc3:commit_check/main.py | grep -A 5 "\-\-message"Repository: commit-check/commit-check
Length of output: 427
Document breaking change in --message flag behavior.
The -m/--message flag changed from accepting an optional file path argument to a boolean flag. Previous usage like -m /path/to/file must now use the positional argument: commit-check -m /path/to/file. Update release notes or migration guide to inform users of this change.
🤖 Prompt for AI Agents
In `@commit_check/main.py` around lines 65 - 66, Document that the semantics of
the -m/--message flag changed from accepting an optional file path to a boolean
store_true flag: update release notes and the migration guide to show the new
usage (use the positional argument for a message file, e.g., commit-check -m
/path/to/file) and add a clear note in the CLI help text for the --message/-m
option in main.py so users know to move prior invocations like `-m
/path/to/file` to the positional parameter.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/main_test.py`:
- Around line 536-549: The test_positional_arg_invalid_commit test can be flaky
because it does not mock commit_check.engine.get_commit_info; update the test to
patch commit_check.engine.get_commit_info (the same way
test_message_validation_with_invalid_commit does) to return a deterministic
commit payload (e.g., a dict with a known "author") before calling main(),
ensuring the validation outcome is independent of the repo state; use the
existing mocking style in other tests (mocker or monkeypatch) to set the return
value and then call main() and assert result == 1.
| @pytest.mark.benchmark | ||
| def test_positional_arg_invalid_commit(self): | ||
| """Test that positional argument correctly rejects invalid commits.""" | ||
| with tempfile.NamedTemporaryFile(mode="w", delete=False) as f: | ||
| f.write("invalid commit message without type") | ||
| f.flush() | ||
|
|
||
| try: | ||
| # Use positional argument with invalid message | ||
| sys.argv = ["commit-check", f.name] | ||
| result = main() | ||
| assert result == 1 # Should fail validation | ||
| finally: | ||
| os.unlink(f.name) |
There was a problem hiding this comment.
Add mock for get_commit_info to ensure consistent test behavior.
This test is missing a mock that similar tests include. Looking at test_message_validation_with_invalid_commit (line 50), it mocks commit_check.engine.get_commit_info to return a known author. Without this mock, the test result could vary depending on the git state of the test environment or if the author happens to match an ignore list entry.
Proposed fix
`@pytest.mark.benchmark`
def test_positional_arg_invalid_commit(self):
"""Test that positional argument correctly rejects invalid commits."""
+ # Note: Consider adding mocker fixture and mocking get_commit_info
+ # similar to test_message_validation_with_invalid_commit
with tempfile.NamedTemporaryFile(mode="w", delete=False) as f:Or convert to use mocker:
`@pytest.mark.benchmark`
- def test_positional_arg_invalid_commit(self):
+ def test_positional_arg_invalid_commit(self, mocker):
"""Test that positional argument correctly rejects invalid commits."""
+ mocker.patch("commit_check.engine.get_commit_info", return_value="test-author")
with tempfile.NamedTemporaryFile(mode="w", delete=False) as f:🤖 Prompt for AI Agents
In `@tests/main_test.py` around lines 536 - 549, The
test_positional_arg_invalid_commit test can be flaky because it does not mock
commit_check.engine.get_commit_info; update the test to patch
commit_check.engine.get_commit_info (the same way
test_message_validation_with_invalid_commit does) to return a deterministic
commit payload (e.g., a dict with a known "author") before calling main(),
ensuring the validation outcome is independent of the repo state; use the
existing mocking style in other tests (mocker or monkeypatch) to set the return
value and then call main() and assert result == 1.
|
@copilot, code review the whole changes again. |
|
@shenxianpeng I've opened a new pull request, #368, to work on those changes. Once the pull request is ready, I'll request review from you. |
fixes #363
Summary by CodeRabbit
New Features
Improvements
Tests