Skip to content
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

fix: skip the header length check and configure header length #68

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 51 additions & 6 deletions src/commitlint/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ def get_args() -> argparse.Namespace:
group.add_argument("--from-hash", type=str, help="From commit hash")
# --to-hash is optional
parser.add_argument("--to-hash", type=str, help="To commit hash", default="HEAD")

# feature options
parser.add_argument(
"--skip-detail",
Expand All @@ -85,6 +84,14 @@ def get_args() -> argparse.Namespace:
default=False,
)

output_group.add_argument(
Copy link
Member

Choose a reason for hiding this comment

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

issue: output_group is for verbose and quiet only.

"--max-header-length", help="commit message header max length"
)
output_group.add_argument(
"--disable-header-length-check",
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: Let's make the length check optional by default and add the option --check-header-length for the header length check.

The length limit is not even on the conventional commit spec.

action="store_true",
help="disable header max length check",
)
# --verbose option is optional
output_group.add_argument(
"-v",
Expand Down Expand Up @@ -152,10 +159,13 @@ def _get_commit_message_from_file(filepath: str) -> str:
return commit_message


# pylint: disable=too-many-arguments
def _handle_commit_message(
commit_message: str,
skip_detail: bool,
hide_input: bool,
max_header_length: int,
disable_max_header_length_check: bool = False,
strip_comments: bool = False,
) -> None:
"""
Expand All @@ -171,7 +181,13 @@ def _handle_commit_message(
Raises:
SystemExit: If the commit message is invalid.
"""
success, errors = lint_commit_message(commit_message, skip_detail, strip_comments)
success, errors = lint_commit_message(
commit_message=commit_message,
skip_detail=skip_detail,
strip_comments=strip_comments,
max_header_length=max_header_length,
disable_max_header_length=disable_max_header_length_check,
)

if success:
console.success(VALIDATION_SUCCESSFUL)
Expand All @@ -182,7 +198,11 @@ def _handle_commit_message(


def _handle_multiple_commit_messages(
commit_messages: List[str], skip_detail: bool, hide_input: bool
commit_messages: List[str],
skip_detail: bool,
hide_input: bool,
disable_max_header_length_check: bool,
max_header_length: int,
) -> None:
"""
Handles multiple commit messages, checks their validity, and prints the result.
Expand All @@ -198,7 +218,12 @@ def _handle_multiple_commit_messages(
has_error = False

for commit_message in commit_messages:
success, errors = lint_commit_message(commit_message, skip_detail)
success, errors = lint_commit_message(
commit_message,
max_header_length=max_header_length,
skip_detail=skip_detail,
disable_max_header_length=disable_max_header_length_check,
)
if success:
console.verbose("lint success")
continue
Expand All @@ -224,6 +249,14 @@ def main() -> None:
config.verbose = args.verbose

console.verbose("starting commitlint")

if args.max_header_length and args.disable_header_length_check:
Copy link
Member

Choose a reason for hiding this comment

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

issue: these can be achieved through argsparse groups.

console.error(
"--max-header-length and "
"--disable-header-length-check can't be passed together"
)
raise CommitlintException

try:
if args.file:
console.verbose("commit message source: file")
Expand All @@ -232,13 +265,19 @@ def main() -> None:
commit_message,
skip_detail=args.skip_detail,
hide_input=args.hide_input,
disable_max_header_length_check=args.disable_header_length_check,
max_header_length=args.max_header_length,
strip_comments=True,
)
elif args.hash:
console.verbose("commit message source: hash")
commit_message = get_commit_message_of_hash(args.hash)
_handle_commit_message(
commit_message, skip_detail=args.skip_detail, hide_input=args.hide_input
commit_message,
skip_detail=args.skip_detail,
hide_input=args.hide_input,
max_header_length=args.max_header_length,
disable_max_header_length_check=args.disable_header_length_check,
)
elif args.from_hash:
console.verbose("commit message source: hash range")
Expand All @@ -249,12 +288,18 @@ def main() -> None:
commit_messages,
skip_detail=args.skip_detail,
hide_input=args.hide_input,
max_header_length=args.max_header_length,
disable_max_header_length_check=args.disable_header_length_check,
)
else:
console.verbose("commit message source: direct message")
commit_message = args.commit_message.strip()
_handle_commit_message(
commit_message, skip_detail=args.skip_detail, hide_input=args.hide_input
commit_message,
skip_detail=args.skip_detail,
hide_input=args.hide_input,
max_header_length=args.max_header_length,
disable_max_header_length_check=args.disable_header_length_check,
)
except CommitlintException as ex:
console.error(f"{ex}")
Expand Down
33 changes: 27 additions & 6 deletions src/commitlint/linter/_linter.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from .. import console
from .utils import is_ignored, remove_comments
from .validators import (
CommitValidator,
HeaderLengthValidator,
PatternValidator,
SimplePatternValidator,
Expand All @@ -16,12 +17,18 @@


def lint_commit_message(
commit_message: str, skip_detail: bool = False, strip_comments: bool = False
commit_message: str,
max_header_length: int,
skip_detail: bool = False,
disable_max_header_length: bool = False,
strip_comments: bool = False,
) -> Tuple[bool, List[str]]:
"""
Lints a commit message.

Args:
disable_max_header_length: flag to disable the max header length check
max_header_length: maximum length of commit message header
commit_message (str): The commit message to be linted.
skip_detail (bool, optional): Whether to skip the detailed error linting
(default is False).
Expand All @@ -47,16 +54,30 @@ def lint_commit_message(
console.verbose("commit message ignored, skipping lint")
return True, []

validator_instances: List[CommitValidator] = []

if not disable_max_header_length:
validator_instances.append(
HeaderLengthValidator(
commit_message=commit_message,
**{"max_header_length": max_header_length}, # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: This will work too.

Suggested change
**{"max_header_length": max_header_length}, # type: ignore
max_header_length=max_header_length,

)
)

# for skip_detail check
if skip_detail:
console.verbose("running simple validators for linting")

validator_instances.append(
SimplePatternValidator(commit_message=commit_message)
)

return run_validators(
commit_message,
validator_classes=[HeaderLengthValidator, SimplePatternValidator],
validator_classes=validator_instances,
fail_fast=True,
)

validator_instances.append(PatternValidator(commit_message=commit_message))

console.verbose("running detailed validators for linting")
return run_validators(
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: I never thought the task was going to be this tough. I have a few alternative suggestions that are effective and require fewer code changes.

However, our tool has a config for storing such configurations. It's already been implemented for verbose and quiet options, you can check that. With that said, the changes will only be on HeaderLengthValidator where the actual validator runs.

commit_message, validator_classes=[HeaderLengthValidator, PatternValidator]
)
return run_validators(validator_classes=validator_instances)
43 changes: 26 additions & 17 deletions src/commitlint/linter/validators.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

import re
from abc import ABC, abstractmethod
from typing import List, Tuple, Type, Union
from typing import Any, Dict, List, Tuple, Union, cast

from .. import console
from ..constants import COMMIT_HEADER_MAX_LENGTH, COMMIT_TYPES
Expand All @@ -30,15 +30,15 @@
class CommitValidator(ABC):
"""Abstract Base validator for commit message."""

def __init__(self, commit_message: str) -> None:
def __init__(self, commit_message: str, **kwargs: Dict[str, Any]) -> None:
self._commit_message = commit_message
self._errors: List[str] = []

# start validation
self.validate()
self.validate(**kwargs)

@abstractmethod
def validate(self) -> None:
def validate(self, **kwargs: Dict[str, Any]) -> None:
"""Performs the validation."""
raise NotImplementedError # pragma: no cover

Expand All @@ -50,6 +50,7 @@ def is_valid(self) -> bool:
"""Checks if there are any errors."""
return len(self._errors) == 0

@property
Copy link
Member

Choose a reason for hiding this comment

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

@property fits perfectly here.

def errors(self) -> List[str]:
"""Get the list of errors."""
return self._errors
Expand All @@ -63,15 +64,23 @@ def commit_message(self) -> str:
class HeaderLengthValidator(CommitValidator):
"""Validator for checking commit header length."""

def validate(self) -> None:
def validate(self, **kwargs: Dict[str, Any]) -> None:
"""
Validates the length of the commit header.

Returns:
None
"""
max_header_length = kwargs.get("max_header_length")

if max_header_length:
header_length = cast(int, max_header_length)
else:
header_length = COMMIT_HEADER_MAX_LENGTH

header = self.commit_message.split("\n")[0]
if len(header) > COMMIT_HEADER_MAX_LENGTH:

if len(header) > int(header_length):
self.add_error(HEADER_LENGTH_ERROR)


Expand All @@ -90,7 +99,7 @@ class SimplePatternValidator(CommitValidator):
r"((\n\n(?P<body>.*))|(\s*))?$"
)

def validate(self) -> None:
def validate(self, **kwargs: Dict[str, Any]) -> None:
"""
Validates the commit message using the regex pattern.

Expand Down Expand Up @@ -118,7 +127,7 @@ class PatternValidator(CommitValidator):
r"(((?P<body>.*))|(\s*))?$"
)

def validate(self) -> None:
def validate(self, **kwargs: Dict[str, Any]) -> None:
"""
Validates the commit message using the regex pattern.

Expand Down Expand Up @@ -286,8 +295,7 @@ def validate_description_no_full_stop_at_end(


def run_validators(
commit_message: str,
validator_classes: List[Type[CommitValidator]],
validator_classes: List[CommitValidator],
fail_fast: bool = False,
) -> Tuple[bool, List[str]]:
"""Runs the provided validators for the commit message.
Expand All @@ -307,17 +315,18 @@ def run_validators(
success = True
errors: List[str] = []

for validator_class in validator_classes:
console.verbose(f"running validator {validator_class.__name__}")
validator = validator_class(commit_message)
if not validator.is_valid():
console.verbose(f"{validator_class.__name__}: validation failed")
for validator_instance in validator_classes:
console.verbose(f"running validator {validator_instance.__class__.__name__}")
if not validator_instance.is_valid():
console.verbose(
f"{validator_instance.__class__.__name__}: validation failed"
)
if fail_fast:
console.verbose(f"fail_fast: {fail_fast}, skipping further validations")
# returning immediately if any error occurs.
return False, validator.errors()
return False, validator_instance.errors

success = False
errors.extend(validator.errors())
errors.extend(validator_instance.errors)

return success, errors
43 changes: 43 additions & 0 deletions tests/test_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -459,3 +459,46 @@ def test__invalid_commit_message_with_hash_range_in_quiet(

mock_stderr_write.assert_not_called()
mock_stdout_write.assert_not_called()


class TestCliHeaderLength:
@patch(
"commitlint.cli.get_args",
return_value=ArgsMock(
commit_message="feat: add method 'formatEnglishDate' \
and 'formatEnglishDateInNepali' (#165)",
quiet=True,
max_header_length=20,
),
)
@patch("sys.stdout.write")
@patch("sys.stderr.write")
def test__main__quiet_option_with_header_max_length(
self, mock_stderr_write, mock_stdout_write, *_
):
with pytest.raises(SystemExit):
main()

mock_stderr_write.assert_not_called()
mock_stdout_write.assert_not_called()

@patch(
"commitlint.cli.get_args",
return_value=ArgsMock(
commit_message="feat: add method 'formatEnglishDate' \
and 'formatEnglishDateInNepali' (#165)",
quiet=True,
max_header_length=20,
disable_header_length_check=True,
),
)
@patch("sys.stdout.write")
@patch("sys.stderr.write")
def test__with_both_header_max_length_and_disabled_max_header_length_check(
self, mock_stderr_write, mock_stdout_write, *_
):
with pytest.raises(CommitlintException):
main()

mock_stderr_write.assert_not_called()
mock_stdout_write.assert_not_called()
Loading
Loading