fix: skip the header length check and configure header length #68
Conversation
golesuman
force-pushed
the
fix/char-length-limit-and-skip-length-check
branch
from
8fc9baa to
3a4539f
Compare
|
Hi @golesuman, Thanks for your contribution. Any updates on this PR? I haven't reviewed the PR since it's in the draft. |
Hi @aj3sh. Could you review if the approach is okay ? It looks kinda hacky to be honest. I couldn't update fixtures to actually test the dynamic max header length as I mentioned above in the additional notes because of my schedule. If it looks good then we can actually do something about the test part :) |
There was a problem hiding this comment.
I have reviewed the current changes and will look into the issue regarding the header length error.
Additionally, if the header length check is not disabled by default, the issue will remain unresolved since no changes have been made to the GitHub Actions. Therefore, I suggest disabling the header length check by default. The changes to GitHub Actions can be addressed later in this case.
| "--max-header-length", help="commit message header max length" | ||
| ) | ||
| output_group.add_argument( | ||
| "--disable-header-length-check", |
There was a problem hiding this comment.
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.
| @@ -85,6 +84,14 @@ def get_args() -> argparse.Namespace: | |||
| default=False, | |||
| ) | |||
|
|
|||
| output_group.add_argument( | |||
There was a problem hiding this comment.
issue: output_group is for verbose and quiet only.
| @@ -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: | |||
There was a problem hiding this comment.
issue: these can be achieved through argsparse groups.
| validator_instances.append( | ||
| HeaderLengthValidator( | ||
| commit_message=commit_message, | ||
| **{"max_header_length": max_header_length}, # type: ignore |
There was a problem hiding this comment.
suggestion: This will work too.
| **{"max_header_length": max_header_length}, # type: ignore | |
| max_header_length=max_header_length, |
| @@ -50,6 +50,7 @@ def is_valid(self) -> bool: | |||
| """Checks if there are any errors.""" | |||
| return len(self._errors) == 0 | |||
|
|
|||
| @property | |||
| console.verbose("running detailed validators for linting") | ||
| return run_validators( |
There was a problem hiding this comment.
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.
Description
Allow the users to make the header length configurable and also allow disable header length check using a flag.
Introduced two flags
--max-header-length--disable-header-length-checkRelated Issue
Fixes #67
Type of Change
Please mark the appropriate option below to describe the type of change your pull request introduces:
Checklist
Examples:
"fix: Fixed foobar bug","feat(accounts): Added foobar feature".README.md.Additional Notes
There is one issue with HEADER_LENGTH_ERROR be cause of COMMIT_HEADER_MAX_LENGTH constant.
I tried to set "%s" in the message but it seems to fail the tests because of fixers LINTER_FIXTURE_PARAMS and we need some way to pass the value to this fixture .
By submitting this pull request, I confirm that I have read and complied with the contribution guidelines of this project.