Actionable comments posted: 7

pyproject.toml (1)

71-75: Optional: pin pytest minimum version for pyproject-based config.

Small guard to avoid older pytest parsing differences.

Apply this diff:

 [tool.pytest.ini_options]
+# Ensure a modern pytest that fully supports pyproject config
+minversion = "7.0"
 # Silence PytestUnknownMarkWarning for custom marks used in tests
 markers = [
     "benchmark: performance-related tests (no-op marker in this project)",
 ]

commit_check/author.py (1)

28-31: Optional: assert known author check types.

Preempt accidental misuse and avoid calling git with an empty format string.

Apply this diff:

 def _get_author_value(check_type: str) -> str:
     """Fetch the author value from git for the given check type."""
-    format_str = _AUTHOR_FORMAT_MAP.get(check_type, "")
+    format_str = _AUTHOR_FORMAT_MAP.get(check_type, "")
+    if not format_str:  # defensive
+        return ""
     return str(get_commit_info(format_str))

commit_check/branch.py (2)

33-39: Optional: tolerate missing 'error'/'suggest' keys.

Use .get with defaults to avoid KeyError on partial configs.

Apply this diff:

-    print_error_message(
-        check['check'], regex,
-        check['error'], branch_name,
-    )
-    if check.get('suggest'):
-        print_suggestion(check['suggest'])
+    print_error_message(
+        check.get('check', 'branch'), regex,
+        check.get('error', ''), branch_name,
+    )
+    if check.get('suggest'):
+        print_suggestion(check['suggest'])

---

71-77: Optional: same defensive .get usage for merge_base.

Apply this diff:

-    print_error_message(
-        merge_check['check'], regex,
-        merge_check['error'], current_branch,
-    )
-    if merge_check.get('suggest'):
-        print_suggestion(merge_check['suggest'])
+    print_error_message(
+        merge_check.get('check', 'merge_base'), regex,
+        merge_check.get('error', ''), current_branch,
+    )
+    if merge_check.get('suggest'):
+        print_suggestion(merge_check['suggest'])

commit_check/commit.py (1)

30-36: Centralize _find_check to avoid duplication across modules

This duplicates commit_check/author.py::_find_check. Consider moving a single find_check helper to commit_check.util and importing it here and in other modules.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between dc169ba and 4e14363.

• commit_check/author.py (1 hunks)
• commit_check/branch.py (2 hunks)
• commit_check/commit.py (1 hunks)
• pyproject.toml (1 hunks)

Learnt from: CR
PR: commit-check/commit-check#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-07T15:16:47.288Z
Learning: Ensure author information in commits is valid: proper author name and email formats

Applied to files:

• commit_check/author.py

commit_check/util.py (5)

• get_commit_info (48-67)
• has_commits (33-46)
• print_error_header (128-144)
• print_error_message (147-159)
• print_suggestion (162-173)

commit_check/main.py (1)

• main (106-136)

commit_check/util.py (5)

• get_branch_name (15-30)
• print_error_header (128-144)
• print_error_message (147-159)
• print_suggestion (162-173)
• git_merge_base (70-84)

tests/branch_test.py (5)

• TestCheckBranch (10-120)
• test_check_merge_base_with_different_check (144-152)
• TestCheckMergeBase (123-169)
• test_check_branch_with_different_check (49-66)
• test_check_merge_base_with_empty_regex (133-141)

commit_check/author.py (1)

• _find_check (20-25)

commit_check/util.py (5)

• print_error_header (128-144)
• print_error_message (147-159)
• print_suggestion (162-173)
• has_commits (33-46)
• get_commit_info (48-67)

commit_check/main.py (1)

• main (106-136)

tests/commit_test.py (2)

• test_check_imperative_different_check_type (348-362)
• test_check_imperative_empty_checks (382-393)

• GitHub Check: install (3.13, windows-latest)
• GitHub Check: install (3.11, windows-latest)
• GitHub Check: install (3.10, windows-latest)
• GitHub Check: Run benchmarks

pyproject.toml (1)

71-75: Registering the custom pytest marker looks good.

This silences PytestUnknownMarkWarning and documents the benchmark mark.

commit_check/commit.py (7)

38-43: LGTM: robust fallback for commit message file

Helper cleanly normalizes empty input to default path.

---

58-66: LGTM: early exit and clear skip messaging when regex missing

Flow is simple and avoids unnecessary I/O.

---

67-75: LGTM: standardized failure path via _print_failure

Good reuse and early return structure.

---

81-89: LGTM: signoff check retrieval and empty-regex skip

Matches the new pattern used across modules.

---

90-99: LGTM: merge commits correctly skipped by subject prefix

Simple and correct.

---

115-116: LGTM: no-commit fast path

Avoids running checks in fresh repos.

---

117-137: LGTM: imperative check flow and conventional-commit description extraction

The colon-split heuristic is clear and keeps the function concise.

Actionable comments posted: 3

commit_check/branch.py (1)

4-4: Python 3.9 compat: imported _find_check likely uses PEP 604 unions

util._find_check is annotated as dict | None in the repo snippets; this breaks on Python 3.9. Please switch to Optional[dict] in commit_check/util.py (and elsewhere) or drop the annotation.

Run to locate offenders:

#!/bin/bash
# Find PEP 604 unions that will fail on Python 3.9
rg -nP --type=py '\b[A-Za-z_][A-Za-z0-9_\[\],\s]*\|\s*None\b|->\s*[A-Za-z_][A-Za-z0-9_\[\],\s]*\|\s*None\b' -C2

commit_check/author.py (1)

26-31: Harden failure printing: guard header flag and avoid KeyError

Use getattr to avoid AttributeError and .get() to avoid KeyError if config misses fields.

 def _print_failure(check: dict, regex: str, actual: str) -> None:
-    if not print_error_header.has_been_called:
-        print_error_header()
-    print_error_message(check['check'], regex, check['error'], actual)
+    if not getattr(print_error_header, "has_been_called", False):
+        print_error_header()
+    print_error_message(str(check.get('check', "")), regex, str(check.get('error', "")), actual)
     if check.get('suggest'):
         print_suggestion(check['suggest'])

commit_check/commit.py (2)

20-26: Unify failure handling: guard header flag and avoid KeyError

Mirror the robustness used elsewhere.

 def _print_failure(check: dict, regex: str, actual: str) -> None:
-    if not print_error_header.has_been_called:
-        print_error_header()  # pragma: no cover
-    print_error_message(check['check'], regex, check['error'], actual)
+    if not getattr(print_error_header, "has_been_called", False):
+        print_error_header()  # pragma: no cover
+    print_error_message(str(check.get('check', "")), regex, str(check.get('error', "")), actual)
     if check.get('suggest'):
         print_suggestion(check['suggest'])

---

90-95: Micro-opt: defer git hash lookup until failure

Avoids a subprocess on success path.

-    commit_hash = get_commit_info("H")
     if re.search(regex, commit_msg):
         return PASS
 
-    _print_failure(check, regex, commit_hash)
+    commit_hash = get_commit_info("H")
+    _print_failure(check, regex, commit_hash)
     return FAIL

commit_check/branch.py (4)

22-24: Polish the skip message

More concise and consistent wording.

-        print(
-            f"{YELLOW}Not found regex for branch naming. skip checking.{RESET_COLOR}",
-        )
+        print(f"{YELLOW}No branch regex configured; skipping check.{RESET_COLOR}")

---

28-29: Consider fullmatch for exact branch rule compliance

re.match allows partial matches unless the pattern is anchored; fullmatch enforces whole-string validation.

-    if re.match(regex, branch_name):
+    if re.fullmatch(regex, branch_name):
         return PASS

---

49-55: Polish the merge_base skip message

Clearer wording.

-    if regex == "":
-        print(
-            f"{YELLOW}Not found target branch for checking merge base. skip checking.{RESET_COLOR}",
-        )
+    if regex == "":
+        print(f"{YELLOW}No target branch configured for merge_base; skipping check.{RESET_COLOR}")
         return PASS

---

56-56: Avoid false positives when detecting existing origin/ prefix

Use startswith to prevent accidental matches where "origin/" appears elsewhere in the string.

-    target_branch = regex if "origin/" in regex else f"origin/{regex}"
+    target_branch = regex if regex.startswith("origin/") else f"origin/{regex}"

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between 313d2ea and 6d4fff6.

• commit_check/author.py (1 hunks)
• commit_check/branch.py (2 hunks)
• commit_check/commit.py (2 hunks)
• commit_check/util.py (1 hunks)
• pyproject.toml (1 hunks)

tests/branch_test.py (4)

• TestCheckBranch (10-120)
• test_check_branch_with_empty_checks (32-46)
• test_check_branch_with_different_check (49-66)
• test_check_branch_with_result_none (93-120)

tests/commit_test.py (1)

• test_check_commit_with_different_check (70-81)

commit_check/util.py (6)

• get_commit_info (56-75)
• has_commits (41-54)
• print_error_header (136-152)
• print_error_message (155-167)
• print_suggestion (170-181)
• _find_check (15-20)

tests/author_test.py (4)

• test_check_author_with_different_check (73-90)
• test_check_author_with_different_check (188-205)
• test_check_author (16-33)
• test_check_author (151-168)

commit_check/util.py (7)

• _find_check (15-20)
• get_branch_name (23-38)
• git_merge_base (78-92)
• print_error_header (136-152)
• print_error_message (155-167)
• print_suggestion (170-181)
• has_commits (41-54)

commit_check/author.py (1)

• _print_failure (26-31)

commit_check/commit.py (1)

• _print_failure (20-25)

tests/branch_test.py (4)

• TestCheckBranch (10-120)
• test_check_merge_base_with_different_check (144-152)
• TestCheckMergeBase (123-169)
• test_check_merge_base_with_empty_regex (133-141)

noxfile.py (1)

• commit_check (45-47)

commit_check/util.py (7)

• _find_check (15-20)
• cmd_output (95-109)
• get_commit_info (56-75)
• print_error_header (136-152)
• print_error_message (155-167)
• print_suggestion (170-181)
• has_commits (41-54)

commit_check/author.py (1)

• _print_failure (26-31)

commit_check/branch.py (1)

• _print_failure (7-12)

commit_check/main.py (1)

• main (106-136)

tests/commit_test.py (5)

• test_check_commit_with_different_check (70-81)
• test_check_imperative_skip_merge_commit (329-344)
• test_check_imperative_empty_checks (382-393)
• test_check_imperative_different_check_type (348-362)
• test_check_commit_signoff_still_fails_non_merge_without_signoff (226-250)

• GitHub Check: install (3.13, windows-latest)
• GitHub Check: install (3.12, windows-latest)
• GitHub Check: install (3.13, ubuntu-24.04)
• GitHub Check: install (3.10, windows-latest)
• GitHub Check: install (3.11, windows-latest)
• GitHub Check: install (3.10, macos-latest)
• GitHub Check: install (3.9, windows-latest)
• GitHub Check: install (3.9, macos-latest)
• GitHub Check: Run benchmarks

pyproject.toml (1)

71-75: Pytest marker config: LGTM

Silences PytestUnknownMarkWarning cleanly without side effects.

commit_check/author.py (1)

39-47: Early-return flow for empty regex: LGTM

Skips I/O and regex work when config lacks a pattern; message is clear.

commit_check/commit.py (2)

60-65: Message regex check path: LGTM

Single lookup + early return reduces branching and work done.

---

110-126: Imperative mood check refactor: LGTM

Clear skip conditions and focused subject parsing keep complexity low.

commit_check/branch.py (1)

16-19: Nice complexity reduction via early return

Using _find_check and bailing out early simplifies control flow.

Pull Request Overview

This PR refactors the commit-check codebase to reduce cognitive complexity by centralizing validation logic and improving code organization. The refactoring introduces helper functions to eliminate repetitive code patterns and standardizes error handling across different validation modules.

Key changes:

• Centralized check finding and error printing logic in utility functions
• Simplified validation functions by removing repetitive loops and conditional structures
• Updated test imports to reflect relocated functions and standardized test markers

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Actionable comments posted: 1

commit_check/commit.py (2)

13-18: Replace PEP 604 str | None with Optional[str] or bump supported Python

Project metadata previously targeted Python >=3.9; X | None requires 3.10+. Use Optional[str] to retain 3.9 support, or update requires-python to >=3.10.

+from typing import Optional
@@
-def _ensure_msg_file(commit_msg_file: str | None) -> str:
+def _ensure_msg_file(commit_msg_file: Optional[str]) -> str:

#!/bin/bash
# Confirm declared Python version to decide between Optional vs. 3.10+
rg -n "requires-python|Programming Language :: Python ::" -S pyproject.toml || true

---

55-57: Guard header gating inside util._print_failure to avoid AttributeError

current implementation in util reads print_error_header.has_been_called directly, which can crash on first call. Use getattr/setattr there.

Apply in commit_check/util.py (for reference):

-    if not print_error_header.has_been_called:
-        print_error_header()
+    if not getattr(print_error_header, "has_been_called", False):
+        print_error_header()  # pragma: no cover
+        setattr(print_error_header, "has_been_called", True)

Also applies to: 86-87, 116-118

tests/commit_test.py (1)

52-55: Use PASS constant instead of magic number

Avoid asserting literal 0; assert against PASS for clarity and resilience.

-    assert result == 0
+    assert result == PASS

tests/author_test.py (1)

260-261: Remove duplicate assertion

The second identical assert is redundant.

-            assert m_print_suggestion.call_count == 1

commit_check/commit.py (3)

26-34: Tighten typing and drop unnecessary str()

Add a type for commit_msg_file; no need to wrap concatenated string with str().

-def read_commit_msg(commit_msg_file) -> str:
+def read_commit_msg(commit_msg_file: str) -> str:
@@
-    except FileNotFoundError:
-        # Commit message is composed by subject and body
-        return str(get_commit_info("s") + "\n\n" + get_commit_info("b"))
+    except FileNotFoundError:
+        # Fallback to subject + body when file is absent
+        return get_commit_info("s") + "\n\n" + get_commit_info("b")

---

35-57: Reduce noise: show subject in failure instead of full message

Printing the entire message can be verbose. Passing the subject keeps output focused.

-    if re.match(regex, commit_msg):
+    if re.match(regex, commit_msg):
         return PASS
-
-    _print_failure(check, regex, commit_msg)
+
+    subject = commit_msg.split('\n')[0].strip()
+    _print_failure(check, regex, subject)
     return FAIL

---

82-87: Avoid unnecessary git calls on success path

Only fetch commit hash if the regex fails.

-    commit_hash = get_commit_info("H")
-    if re.search(regex, commit_msg):
-        return PASS
-
-    _print_failure(check, regex, commit_hash)
-    return FAIL
+    if re.search(regex, commit_msg):
+        return PASS
+
+    commit_hash = get_commit_info("H")
+    _print_failure(check, regex, commit_hash)
+    return FAIL

commit_check/branch.py (3)

20-21: Prefer fullmatch for branch naming rules.

re.match allows partial prefixes; most naming policies expect the whole branch to conform.

-    if re.match(regex, branch_name):
+    if re.fullmatch(regex, branch_name):
         return PASS

---

42-47: Consistent warning mechanism.

Consider routing warnings through a shared helper (e.g., util.print_warning(...)) to keep messaging uniform across modules and simplify testing.

---

48-49: Nit: use startswith for remote prefix.

"origin/" in regex can misfire on exotic patterns; regex.startswith("origin/") is stricter.

-    target_branch = regex if "origin/" in regex else f"origin/{regex}"
+    target_branch = regex if regex.startswith("origin/") else f"origin/{regex}"

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between 6d4fff6 and bfb2edb.

• commit_check/author.py (1 hunks)
• commit_check/branch.py (2 hunks)
• commit_check/commit.py (2 hunks)
• commit_check/util.py (1 hunks)
• tests/author_test.py (2 hunks)
• tests/branch_test.py (2 hunks)
• tests/commit_test.py (1 hunks)

• commit_check/util.py
• commit_check/author.py

tests/util_test.py (2)

• TestUtil (16-323)
• TestGetCommitInfo (116-180)

noxfile.py (1)

• commit_check (45-47)

commit_check/util.py (5)

• _find_check (15-20)
• _print_failure (23-33)
• get_branch_name (36-51)
• git_merge_base (91-105)
• has_commits (54-67)

noxfile.py (1)

• commit_check (45-47)

commit_check/util.py (5)

• _find_check (15-20)
• _print_failure (23-33)
• cmd_output (108-122)
• get_commit_info (69-88)
• has_commits (54-67)

commit_check/main.py (1)

• main (106-136)

• GitHub Check: install (3.12, windows-latest)
• GitHub Check: install (3.13, windows-latest)
• GitHub Check: install (3.11, windows-latest)
• GitHub Check: install (3.10, macos-latest)
• GitHub Check: install (3.10, windows-latest)
• GitHub Check: install (3.9, macos-latest)
• GitHub Check: Run benchmarks

tests/commit_test.py (1)

8-8: LGTM: patching util-level printers

Switching LOCATION to "commit_check.util" aligns with the refactor.

tests/author_test.py (2)

134-138: LGTM: patch print_error_message/print_suggestion at util-level

Targets match the refactor centralizing output.

---

249-253: LGTM: consistent util-level patches here as well

Matches the new design.

tests/branch_test.py (1)

110-114: Mocks redirected to util — correct alignment.

Patching commit_check.util.print_error_message and print_suggestion matches the centralization. LGTM.

commit_check/branch.py (1)

8-24: Early-return refactor reads well and reduces complexity.

Centralized lookup/printing and short-circuiting paths look good.