diff --git a/README.rst b/README.rst index a8b0e478..36e0d5d2 100644 --- a/README.rst +++ b/README.rst @@ -177,8 +177,8 @@ Check Branch Naming Failed Type branch check failed ==> test-branch It doesn't match regex: ^(feature|bugfix|hotfix|release|chore|feat|fix)\/.+|(master)|(main)|(HEAD)|(PR-.+) - The branch should follow Conventional Branch. See https://conventional-branches.github.io/ - Suggest: git checkout -b / + The branch should follow Conventional Branch. See https://conventional-branch.github.io/ + Suggest: Use / with allowed types or ignore_authors in config branch section to bypass Check Commit Signature Failed diff --git a/cchk.toml b/cchk.toml index 08ffcbf8..c6818caf 100644 --- a/cchk.toml +++ b/cchk.toml @@ -13,7 +13,6 @@ allow_fixup_commits = true allow_wip_commits = false require_body = false require_signed_off_by = false -allow_authors = [] ignore_authors = ["dependabot[bot]", "copilot[bot]"] [branch] @@ -21,3 +20,4 @@ ignore_authors = ["dependabot[bot]", "copilot[bot]"] conventional_branch = true allow_branch_types = ["feature", "bugfix", "hotfix", "release", "chore", "feat", "fix"] require_rebase_target = "main" +ignore_authors = ["dependabot[bot]", "copilot[bot]"] diff --git a/commit_check/engine.py b/commit_check/engine.py index 3f17b3e5..e4f39271 100644 --- a/commit_check/engine.py +++ b/commit_check/engine.py @@ -4,6 +4,7 @@ from abc import ABC, abstractmethod from dataclasses import dataclass from enum import IntEnum +from dataclasses import field from commit_check.rule_builder import ValidationRule from commit_check.util import ( @@ -28,6 +29,7 @@ class ValidationContext: stdin_text: Optional[str] = None commit_file: Optional[str] = None + config: Dict = field(default_factory=dict) class BaseValidator(ABC): @@ -42,7 +44,45 @@ def validate(self, context: ValidationContext) -> ValidationResult: pass def _should_skip_validation(self, context: ValidationContext) -> bool: - """Determine if validation should be skipped.""" + """ + Determine if validation should be skipped. + + Skip only when there is no stdin_text, no commit_file, and no commits. + """ + return ( + context.stdin_text is None + and context.commit_file is None + and not has_commits() + ) + + def _should_skip_commit_validation(self, context: ValidationContext) -> bool: + """ + Determine if commit validation should be skipped. + + Skip if the current author is in the ignore_authors list for commits, + or if no stdin_text, no commit_file, and no commits exist. + """ + ignore_authors = context.config.get("commit", {}).get("ignore_authors", []) + current_author = get_commit_info("an") + if current_author and current_author in ignore_authors: + return True + return ( + context.stdin_text is None + and context.commit_file is None + and not has_commits() + ) + + def _should_skip_branch_validation(self, context: ValidationContext) -> bool: + """ + Determine if branch validation should be skipped. + + Skip if the current author is in the ignore_authors list for branches, + or if no stdin_text and no commits exist. + """ + ignore_authors = context.config.get("branch", {}).get("ignore_authors", []) + current_author = get_commit_info("an") + if current_author and current_author in ignore_authors: + return True return context.stdin_text is None and not has_commits() def _print_failure(self, actual_value: str, regex_or_constraint: str = "") -> None: @@ -58,7 +98,7 @@ class CommitMessageValidator(BaseValidator): """Validates commit messages against conventional commit standards.""" def validate(self, context: ValidationContext) -> ValidationResult: - if self._should_skip_validation(context): + if self._should_skip_commit_validation(context): return ValidationResult.PASS message = self._get_commit_message(context) @@ -95,7 +135,7 @@ class SubjectValidator(BaseValidator): """Validates commit subject lines.""" def validate(self, context: ValidationContext) -> ValidationResult: - if self._should_skip_validation(context): + if self._should_skip_commit_validation(context): return ValidationResult.PASS subject = self._get_subject(context) @@ -198,7 +238,8 @@ class AuthorValidator(BaseValidator): """Validates author information.""" def validate(self, context: ValidationContext) -> ValidationResult: - if self._should_skip_validation(context): + # Use commit skip logic for ignore_authors + if self._should_skip_commit_validation(context): return ValidationResult.PASS author_value = self._get_author_value(context) @@ -243,6 +284,8 @@ class BranchValidator(BaseValidator): """Validates branch names.""" def validate(self, context: ValidationContext) -> ValidationResult: + if self._should_skip_branch_validation(context): + return ValidationResult.PASS branch_name = ( context.stdin_text.strip() if context.stdin_text else get_branch_name() ) @@ -263,7 +306,7 @@ class MergeBaseValidator(BaseValidator): """Validates merge base ancestry.""" def validate(self, context: ValidationContext) -> ValidationResult: - if not has_commits(): + if self._should_skip_branch_validation(context): return ValidationResult.PASS current_branch = get_branch_name() @@ -347,7 +390,7 @@ class BodyValidator(BaseValidator): """Validates that commit messages contain a body when required.""" def validate(self, context: ValidationContext) -> ValidationResult: - if self._should_skip_validation(context): + if self._should_skip_commit_validation(context): return ValidationResult.PASS message = self._get_commit_message(context) @@ -395,7 +438,7 @@ class CommitTypeValidator(BaseValidator): """Base validator for special commit types (merge, revert, fixup, WIP, empty).""" def validate(self, context: ValidationContext) -> ValidationResult: - if self._should_skip_validation(context): + if self._should_skip_commit_validation(context): return ValidationResult.PASS message = self._get_commit_message(context) @@ -483,8 +526,6 @@ class ValidationEngine: "subject_min_length": SubjectLengthValidator, "author_name": AuthorValidator, "author_email": AuthorValidator, - "allow_authors": AuthorValidator, - "ignore_authors": AuthorValidator, "branch": BranchValidator, "merge_base": MergeBaseValidator, "require_signed_off_by": SignoffValidator, @@ -494,6 +535,7 @@ class ValidationEngine: "allow_empty_commits": CommitTypeValidator, "allow_fixup_commits": CommitTypeValidator, "allow_wip_commits": CommitTypeValidator, + "ignore_authors": CommitTypeValidator, } def __init__(self, rules: List[ValidationRule]): diff --git a/commit_check/main.py b/commit_check/main.py index 7fbeb626..16f13952 100644 --- a/commit_check/main.py +++ b/commit_check/main.py @@ -211,6 +211,7 @@ def main() -> int: context = ValidationContext( stdin_text=stdin_content, commit_file=commit_file_path, + config=config_data, ) # Run validation diff --git a/commit_check/rule_builder.py b/commit_check/rule_builder.py index 56a115c2..a25f2349 100644 --- a/commit_check/rule_builder.py +++ b/commit_check/rule_builder.py @@ -100,8 +100,6 @@ def _build_single_rule( return self._build_length_rule(catalog_entry, "subject_max_length") elif check == "subject_min_length": return self._build_length_rule(catalog_entry, "subject_min_length") - elif check == "allow_authors": - return self._build_author_list_rule(catalog_entry, "allow_authors") elif check == "ignore_authors": return self._build_author_list_rule(catalog_entry, "ignore_authors") elif check == "merge_base": @@ -174,15 +172,9 @@ def _build_author_list_rule( if not isinstance(author_list, list) or not author_list: return None - if config_key == "allow_authors": - return ValidationRule( - check=catalog_entry.check, - error=catalog_entry.error, - suggest=catalog_entry.suggest, - allowed=author_list, - ) - else: # ignore_authors + if config_key == "ignore_authors": return ValidationRule(check=catalog_entry.check, ignored=author_list) + return None def _build_merge_base_rule( self, catalog_entry: RuleCatalogEntry diff --git a/commit_check/rules_catalog.py b/commit_check/rules_catalog.py index 56b99b41..64af216c 100644 --- a/commit_check/rules_catalog.py +++ b/commit_check/rules_catalog.py @@ -92,12 +92,6 @@ class RuleCatalogEntry: error="The committer's email seems invalid", suggest="git config user.email yourname@example.com", ), - RuleCatalogEntry( - check="allow_authors", - regex=None, - error="Author is not allowed", - suggest="Use a configured author or adjust configuration", - ), RuleCatalogEntry( check="ignore_authors", regex=None, @@ -117,8 +111,8 @@ class RuleCatalogEntry: RuleCatalogEntry( check="branch", regex=None, # Built dynamically from config - error="The branch should follow Conventional Branch. See https://conventional-branches.github.io/", - suggest="git checkout -b /", + error="The branch should follow Conventional Branch. See https://conventional-branch.github.io/", + suggest="Use / with allowed types or ignore_authors in config branch section to bypass", ), RuleCatalogEntry( check="merge_base", @@ -126,4 +120,10 @@ class RuleCatalogEntry: error="Current branch is not rebased onto target branch", suggest="Rebase or merge with the target branch", ), + RuleCatalogEntry( + check="ignore_authors", + regex=None, + error=None, + suggest=None, + ), ] diff --git a/docs/configuration.rst b/docs/configuration.rst index 842de4f5..5d2bff65 100644 --- a/docs/configuration.rst +++ b/docs/configuration.rst @@ -36,7 +36,6 @@ Example Configuration allow_fixup_commits = true allow_wip_commits = false require_body = false - # allow_authors = [] # Optional - all authors allowed by default # ignore_authors = [] # Optional - no authors ignored by default require_signed_off_by = false # required_signoff_name = "Your Name" # Optional @@ -47,6 +46,7 @@ Example Configuration conventional_branch = true allow_branch_types = ["feature", "bugfix", "hotfix", "release", "chore", "feat", "fix"] # require_rebase_target = "main" # Optional - no rebase requirement by default + # ignore_authors = [] # Optional - no authors ignored by default Options Table Description @@ -120,11 +120,6 @@ Options Table Description - bool - false - Require a body in the commit message. - * - commit - - allow_authors - - list[str] - - [] (all allowed) - - List of allowed authors. If empty, all authors are allowed except those in ignore_authors. * - commit - ignore_authors - list[str] @@ -150,3 +145,8 @@ Options Table Description - str - None (no requirement) - Target branch for rebase requirement. If not set, no rebase validation is performed. + * - branch + - ignore_authors + - list[str] + - [] (none ignored) + - List of authors to ignore (i.e., always allow). diff --git a/docs/migration.rst b/docs/migration.rst index b91d50a0..2c662792 100644 --- a/docs/migration.rst +++ b/docs/migration.rst @@ -115,7 +115,6 @@ YAML (v1.x) vs TOML (v2.0+) allow_wip_commits = false require_body = false require_signed_off_by = false - allow_authors = [] ignore_authors = ["dependabot[bot]", "copilot[bot]"] [branch] diff --git a/docs/what-is-new.rst b/docs/what-is-new.rst index 893a345f..29f7fabb 100644 --- a/docs/what-is-new.rst +++ b/docs/what-is-new.rst @@ -137,12 +137,10 @@ Flexible author validation with allow/ignore lists. [commit] # Built-in validation with sensible defaults for author name/email - # Optional: restrict to specific authors - allow_authors = ["John Doe ", "Jane Smith "] # Optional: ignore specific authors (e.g., bots) ignore_authors = ["dependabot[bot]", "renovate[bot]"] -**Benefits**: Built-in validation patterns, flexible allow/ignore lists, automatic bot detection. +**Benefits**: Built-in validation patterns, flexible ignore lists, automatic bot detection. Signed-off-by Requirements ^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/tests/engine_comprehensive_test.py b/tests/engine_comprehensive_test.py index a8d3f6ca..256e3453 100644 --- a/tests/engine_comprehensive_test.py +++ b/tests/engine_comprehensive_test.py @@ -237,8 +237,6 @@ def test_validation_engine_validator_map(self): "subject_min_length": SubjectLengthValidator, "author_name": AuthorValidator, "author_email": AuthorValidator, - "allow_authors": AuthorValidator, - "ignore_authors": AuthorValidator, "branch": BranchValidator, "merge_base": MergeBaseValidator, "require_signed_off_by": SignoffValidator, @@ -248,6 +246,7 @@ def test_validation_engine_validator_map(self): "allow_empty_commits": CommitTypeValidator, "allow_fixup_commits": CommitTypeValidator, "allow_wip_commits": CommitTypeValidator, + "ignore_authors": CommitTypeValidator, } for check, validator_class in expected_mappings.items(): diff --git a/tests/engine_test.py b/tests/engine_test.py index c88e4946..45edb06e 100644 --- a/tests/engine_test.py +++ b/tests/engine_test.py @@ -101,6 +101,7 @@ def test_commit_message_validator_file_not_found(self, mock_get_commit_info): mock_get_commit_info.side_effect = lambda format_str: { "s": "invalid commit message", "b": "", + "an": "author", }[format_str] rule = ValidationRule(check="message", regex=r"^feat:") @@ -125,65 +126,108 @@ def test_commit_message_validator_from_git(self, mock_get_commit_info): result = validator.validate(context) assert result == ValidationResult.PASS - # Should call get_commit_info twice: once for subject, once for body - assert mock_get_commit_info.call_count == 2 + # Should call get_commit_info three times: subject, body, and author + assert mock_get_commit_info.call_count == 3 class TestBranchValidator: + @patch("commit_check.engine.has_commits") @patch("commit_check.engine.get_branch_name") - def test_branch_validator_valid_branch(self, mock_get_branch_name): + def test_branch_validator_valid_branch( + self, mock_get_branch_name, mock_has_commits + ): """Test BranchValidator with valid branch name.""" + mock_has_commits.return_value = True mock_get_branch_name.return_value = "feature/new-feature" - rule = ValidationRule(check="branch", regex=r"^(feature|bugfix|hotfix)/.+") validator = BranchValidator(rule) - context = ValidationContext() - + config = {"branch": {"ignore_authors": ["ignored"]}} + context = ValidationContext(config=config) result = validator.validate(context) assert result == ValidationResult.PASS + assert result == ValidationResult.PASS + assert result == ValidationResult.PASS + @patch("commit_check.engine.has_commits") @patch("commit_check.engine.get_branch_name") - def test_branch_validator_invalid_branch(self, mock_get_branch_name): + def test_branch_validator_invalid_branch( + self, mock_get_branch_name, mock_has_commits + ): """Test BranchValidator with invalid branch name.""" + mock_has_commits.return_value = True mock_get_branch_name.return_value = "invalid-branch-name" - rule = ValidationRule(check="branch", regex=r"^(feature|bugfix|hotfix)/.+") validator = BranchValidator(rule) - context = ValidationContext() - + config = {"branch": {"ignore_authors": ["ignored"]}} + context = ValidationContext(config=config) result = validator.validate(context) assert result == ValidationResult.FAIL + @patch("commit_check.engine.get_branch_name") + @patch("commit_check.engine.get_commit_info") + def test_branch_validator_ignored_author( + self, mock_get_commit_info, mock_get_branch_name + ): + """Test BranchValidator skips validation for ignored author.""" + mock_get_branch_name.return_value = "invalid-branch-name" + mock_get_commit_info.return_value = "ignored" + rule = ValidationRule(check="branch", regex=r"^(feature|bugfix|hotfix)/.+") + validator = BranchValidator(rule) + config = {"branch": {"ignore_authors": ["ignored"]}} + context = ValidationContext(config=config) + result = validator.validate(context) + assert result == ValidationResult.PASS + class TestAuthorValidator: + @patch("commit_check.engine.has_commits") @patch("commit_check.engine.get_commit_info") - def test_author_validator_name_valid(self, mock_get_commit_info): + def test_author_validator_name_valid(self, mock_get_commit_info, mock_has_commits): """Test AuthorValidator for author name.""" + mock_has_commits.return_value = True mock_get_commit_info.return_value = "John Doe" - rule = ValidationRule(check="author_name", regex=r"^[A-Z][a-z]+ [A-Z][a-z]+$") validator = AuthorValidator(rule) - context = ValidationContext() - + config = {"commit": {"ignore_authors": ["ignored"]}} + context = ValidationContext(config=config) result = validator.validate(context) assert result == ValidationResult.PASS - mock_get_commit_info.assert_called_once_with("an") + @patch("commit_check.engine.has_commits") @patch("commit_check.engine.get_commit_info") - def test_author_validator_email_valid(self, mock_get_commit_info): + def test_author_validator_email_valid(self, mock_get_commit_info, mock_has_commits): """Test AuthorValidator for author email.""" + mock_has_commits.return_value = True mock_get_commit_info.return_value = "john.doe@example.com" - rule = ValidationRule( check="author_email", regex=r"^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$", ) validator = AuthorValidator(rule) - context = ValidationContext() + config = {"commit": {"ignore_authors": ["ignored"]}} + context = ValidationContext(config=config) + result = validator.validate(context) + assert result == ValidationResult.PASS + # Called once for skip logic ("an"), once for value ("ae") + assert mock_get_commit_info.call_count == 2 + assert mock_get_commit_info.call_args_list[0][0][0] == "an" + assert mock_get_commit_info.call_args_list[1][0][0] == "ae" + assert result == ValidationResult.PASS + # Called once for skip logic ("an"), once for value ("ae") + assert mock_get_commit_info.call_count == 2 + assert mock_get_commit_info.call_args_list[0][0][0] == "an" + assert mock_get_commit_info.call_args_list[1][0][0] == "ae" + @patch("commit_check.engine.get_commit_info") + def test_author_validator_ignored_author(self, mock_get_commit_info): + """Test AuthorValidator skips validation for ignored author.""" + mock_get_commit_info.return_value = "ignored" + rule = ValidationRule(check="author_name", regex=r"^[A-Z][a-z]+ [A-Z][a-z]+$") + validator = AuthorValidator(rule) + config = {"commit": {"ignore_authors": ["ignored"]}} + context = ValidationContext(config=config) result = validator.validate(context) assert result == ValidationResult.PASS - mock_get_commit_info.assert_called_once_with("ae") class TestCommitTypeValidator: diff --git a/tests/rule_builder_test.py b/tests/rule_builder_test.py index 66da895b..66bd88ce 100644 --- a/tests/rule_builder_test.py +++ b/tests/rule_builder_test.py @@ -5,50 +5,6 @@ class TestValidationRule: - def test_validation_rule_creation(self): - """Test ValidationRule creation with all fields.""" - rule = ValidationRule( - check="test_check", - regex=r"^test:", - error="Test error", - suggest="Test suggestion", - value=42, - allowed=["allowed1", "allowed2"], - ignored=["ignored1", "ignored2"], - ) - - assert rule.check == "test_check" - assert rule.regex == r"^test:" - assert rule.error == "Test error" - assert rule.suggest == "Test suggestion" - assert rule.value == 42 - assert rule.allowed == ["allowed1", "allowed2"] - assert rule.ignored == ["ignored1", "ignored2"] - - def test_validation_rule_to_dict_with_allowed(self): - """Test ValidationRule.to_dict with allowed field (line 34).""" - rule = ValidationRule( - check="allow_authors", - regex="", - error="Author not allowed", - suggest="Use allowed author", - allowed=["alice@example.com", "bob@example.com"], - ) - - result = rule.to_dict() - expected = { - "check": "allow_authors", - "regex": "", - "error": "Author not allowed", - "suggest": "Use allowed author", - "allowed": ["alice@example.com", "bob@example.com"], - "allowed_types": [ - "alice@example.com", - "bob@example.com", - ], # Backward compatibility - } - assert result == expected - def test_validation_rule_to_dict_with_ignored(self): """Test ValidationRule.to_dict() method with ignored field.""" rule = ValidationRule(check="test_check", ignored=["ignored1", "ignored2"]) @@ -82,26 +38,6 @@ def test_rule_builder_conventional_branch_disabled(self): rule = builder._build_conventional_branch_rule(catalog_entry) assert rule is None - def test_rule_builder_allow_authors_list(self): - """Test RuleBuilder with allow_authors list (line 176).""" - config = {"commit": {"allow_authors": ["alice@example.com", "bob@example.com"]}} - - builder = RuleBuilder(config) - catalog_entry = RuleCatalogEntry( - check="allow_authors", - regex="", - error="Author not allowed", - suggest="Use allowed author", - ) - - # This should create a rule with allowed authors - rule = builder._build_author_list_rule(catalog_entry, "allow_authors") - assert rule is not None - assert rule.check == "allow_authors" - assert rule.allowed == ["alice@example.com", "bob@example.com"] - assert rule.error == "Author not allowed" - assert rule.suggest == "Use allowed author" - def test_rule_builder_ignore_authors_list(self): """Test RuleBuilder with ignore_authors list.""" config = {"commit": {"ignore_authors": ["spam@example.com", "bot@example.com"]}} @@ -120,43 +56,17 @@ def test_rule_builder_ignore_authors_list(self): assert rule.check == "ignore_authors" assert rule.ignored == ["spam@example.com", "bot@example.com"] - def test_rule_builder_empty_author_list(self): - """Test RuleBuilder with empty author list returns None.""" - config = {"commit": {"allow_authors": []}} - - builder = RuleBuilder(config) - catalog_entry = RuleCatalogEntry( - check="allow_authors", regex="", error="", suggest="" - ) - - # This should return None for empty list - rule = builder._build_author_list_rule(catalog_entry, "allow_authors") - assert rule is None - - def test_rule_builder_missing_author_list(self): - """Test RuleBuilder with missing author list returns None.""" - config = {"commit": {}} - - builder = RuleBuilder(config) - catalog_entry = RuleCatalogEntry( - check="allow_authors", regex="", error="", suggest="" - ) - - # This should return None for missing config - rule = builder._build_author_list_rule(catalog_entry, "allow_authors") - assert rule is None - def test_rule_builder_invalid_author_list_type(self): """Test RuleBuilder with invalid author list type returns None.""" - config = {"commit": {"allow_authors": "not_a_list"}} + config = {"commit": {"ignore_authors": "not_a_list"}} builder = RuleBuilder(config) catalog_entry = RuleCatalogEntry( - check="allow_authors", regex="", error="", suggest="" + check="ignore_authors", regex="", error="", suggest="" ) # This should return None for invalid type - rule = builder._build_author_list_rule(catalog_entry, "allow_authors") + rule = builder._build_author_list_rule(catalog_entry, "ignore_authors") assert rule is None def test_rule_builder_length_rule_with_format(self):