chore: add pre-commit hooks and commitlint configuration #14
Conversation
📝 WalkthroughWalkthroughAdds repository-level automation and linting: a comprehensive pre-commit configuration with multiple hooks and stages, and a new commitlint TypeScript config extending the conventional preset with explicit rules and export. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks✅ Passed checks (3 passed)
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 |
![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
🧹 Nitpick comments (4)
commitlint.config.mjs (2)
33-41: Avoid redundant checks: pick eithersigned-off-byortrailer-existsBoth rules enforce the same trailer; keeping both is redundant. Prefer a single rule (usually
trailer-existsto ensure it’s in the trailer section)."subject-full-stop": [2, "never", "."], - "signed-off-by": [2, "always", "Signed-off-by:"], + // "signed-off-by": [2, "always", "Signed-off-by:"], /* * Enable type if we need, warn it currently. */ "type-enum": [0, "always", ['ci', 'docs', 'feat', 'fix', 'perf', 'refactor', 'revert', 'rfe', 'style']], "type-case": [0, "always", "lower-case"], "type-empty": [0, "never"], "trailer-exists": [2, "always", "Signed-off-by:"]Reference list of supported rules includes both entries. (commitlint.js.org)
37-37: Comment says “warn it currently”, but severity is off (0)If you want warnings (non‑blocking), set severity to 1.
- "type-enum": [0, "always", ['ci', 'docs', 'feat', 'fix', 'perf', 'refactor', 'revert', 'rfe', 'style']], + "type-enum": [1, "always", ['ci', 'docs', 'feat', 'fix', 'perf', 'refactor', 'revert', 'rfe', 'style']],Committed types are enforced by
type-enumwhen enabled. (commitlint.js.org).pre-commit-config.yaml (2)
15-16: FYI:no-commit-to-branchalready protectsmainandmasterby defaultYou can add more branches/patterns via
argsif needed; otherwise defaults are fine.Docs for defaults and args are here. (github.com)
35-41: Optional: pin commitlint dependencies for stricter reproducibilityYou already pin the hook revision. To avoid potential
@commitlint/*version mismatches when adding plugins later, consider pinning versions inadditional_dependencies(e.g.,@commitlint/[email protected]). Not required now, just future‑proofing.Background on dependency mismatches and rules recognition. (commitlint.js.org)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.pre-commit-config.yaml(1 hunks)commitlint.config.mjs(1 hunks)
🔇 Additional comments (5)
commitlint.config.mjs (3)
6-11: Config shape and ESM export look goodUsing ESM with
export default, extending@commitlint/config-conventional, and settingformatter: '@commitlint/format'matches current commitlint docs. (commitlint.js.org)
49-49: Do you want to enforce Signed-off-by on merge commits too?With
defaultIgnores: true, commitlint skips merge commits; that meansSigned-off-by:won’t be required on merges. If you want it enforced everywhere, setdefaultIgnores: false(and add customignoresif needed).Docs detail
ignoresanddefaultIgnores. (commitlint.js.org)
53-55: Nice touch onhelpUrlLinking to Avocado’s commit style guide matches the local policy (72 char header, no trailing period, Signed‑off‑by).
Avocado commit style guide confirms these rules. (avocado-framework.readthedocs.io)
.pre-commit-config.yaml (2)
1-1: Correct staging setup for commitlintInstalling both
pre-commitandcommit-msghook types is the recommended pattern. (github.com)
3-17: Hook versions look currentPinned versions are recent/stable:
pre-commit-hooks v6.0.0,gitleaks v8.28.0,isort 6.0.1,black 25.9.0,codespell v2.4.1,commitlint-pre-commit-hook v9.22.0.Refs: pre-commit-hooks v6.0.0 usage; Gitleaks v8.28.0 release; isort 6.0.1; Black 25.9.0; Codespell 2.4.1; commitlint-pre-commit-hook v9.22.0. (github.com)
There was a problem hiding this comment.
Hi @PaulYuuu, thank you for introducing the pre-commit hooks, this will definitely be an improvement of our static-checks. I have just couple of comments:
- IIUIC this should replace the current static-checks scripts, therefore I would remove them from repo so we won't have two solutions.
- The last missing static check is lint check. Unfortunatly that is higly configurable in static-checks therefore I don't think we can use regular lint hook for it and will need to run the current script via hooks. Something like this:
- repo: local
hooks:
- id: check-lint
name: Run pylint checks
entry: static-checks/check-lint
language: system
types: [python]
pass_filenames: false
| args: ['--maxkb=5120'] | ||
| - id: check-ast | ||
| - id: check-yaml | ||
| exclude: .cirrus.yml |
There was a problem hiding this comment.
The .cirrus.yml is only related to avocado-vt. For example, on avocado file we need to exclude different files for example examples/yaml_to_mux/. If we won't be able to have a separated exclude list for different repos then I think we will have to remove this check.
There was a problem hiding this comment.
Yes, cirrus.yml does not follow the yaml syntax, but it's vt only. So I am okay to remove it from here, if we have this requirement at specific repository, we can use another github action or static-check.
| - id: isort | ||
| args: ['--profile=black'] | ||
| - repo: https://github.com/psf/black | ||
| rev: 25.9.0 |
There was a problem hiding this comment.
Currently, we are using black version 22.3.0, but probably we can do update with this change.
There was a problem hiding this comment.
Thanks for your confirmation. Indeed, IIRC, 22.3.0 is to keep maintain the compatibility of python3.8, as we dropped its support, so I hope we can upgrade some toolchains to align tool's bug fix or style rules.
E.g. 22.3.0 will format multiple string line but has an issue like
# original
test = (
"foo"
"bar"
)
# 22.3.0
test = ("foo" "bar")
# 25.9.0
test = ("foobar")We can consider which version to use, not necessarily the latest version I put here.
There was a problem hiding this comment.
Sure we can upgrade to the latest version
Hello @richtja, I did not add pylint here is because the scope of pylint is not clear for me and I am not sure if we still need it, as we already have black and isort. I can also add pylint pre-commit hook, and for each repo, link the pylintrc from static-check submodule to the repo root, then it should works. |
- Add comprehensive pre-commit configuration with hooks for: - Code quality checks (black, isort, pylint, codespell) - Security scanning (gitleaks) - Line ending and whitespace fixes - Add commitlint configuration with conventional commit format - Configure signed-off-by trailer requirement for commits Signed-off-by: Yihuang Yu <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
commitlint.config.ts (1)
8-10: Consider removing disabled rules for clarity.Multiple rules are explicitly disabled (level
0). If these rules are intentionally not enforced, consider removing them entirely from the configuration to reduce clutter and make the active rules more apparent. If they serve as documentation of available options, this is acceptable but may confuse contributors.Apply this diff to remove the disabled rules:
// Body 'body-leading-blank': [2, 'always'], - 'body-empty': [0, 'never'], - 'body-min-length': [0, 'always', 1], - 'body-case': [0, 'always', 'lower-case'], // Footer 'footer-leading-blank': [2, 'always'], - 'footer-empty': [0, 'never'], - 'footer-max-length': [0, 'always', 72], // Header - 'header-case': [0, 'always', 'lower-case'], 'header-full-stop': [2, 'never', '.'], 'header-max-length': [2, 'always', 72], 'header-min-length': [2, 'always', 1], // Scope / Subject - 'scope-case': [0, 'always', 'lower-case'], - 'subject-case': [0, 'always', 'lower-case'], - 'subject-empty': [0, 'never'], 'subject-full-stop': [2, 'never', '.'],Also applies to: 14-15, 18-18, 24-26
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.pre-commit-config.yaml(1 hunks)commitlint.config.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-29T03:59:46.902Z
Learnt from: PaulYuuu
PR: avocado-framework/avocado-static-checks#14
File: .pre-commit-config.yaml:30-35
Timestamp: 2025-09-29T03:59:46.902Z
Learning: When .pre-commit-config.yaml is in a submodule, it's correct for it to reference files like 'spell.ignore' that will exist in the consuming repositories, not in the submodule itself.
Applied to files:
.pre-commit-config.yaml
🔇 Additional comments (2)
commitlint.config.ts (1)
1-36: LGTM! Configuration correctly enforces commit conventions.The TypeScript configuration properly extends the conventional preset and enforces the key requirements mentioned in the PR objectives: signed-off-by trailer (Line 30) and conventional commit format with appropriate header constraints (Lines 19-21, 27). The structure and typing are correct, and the helpUrl appropriately points to the contributor style guide.
.pre-commit-config.yaml (1)
1-45: Comprehensive pre-commit configuration with proper commitlint integration.The configuration provides a well-rounded set of hooks covering code quality (black, isort, pylint), security (gitleaks, detect-private-key), file validation (check-yaml, check-ast), and commit message validation (commitlint). The commitlint hook correctly references the commitlint.config.ts file and uses the appropriate commit-msg stage.
Key strengths:
- Lines 40-45: Commitlint integration properly adds @commitlint/config-conventional dependency and will auto-discover commitlint.config.ts
- Line 26: isort configured with black profile ensures formatter compatibility
- Lines 7-8: Large file check with 5MB limit prevents accidental commits of binaries
- Lines 14-15: Line ending normalization enforces LF across the codebase
Based on learnings: The spell.ignore reference (Line 39) is correct since this is a submodule configuration and the file will exist in consuming repositories.
| - repo: https://github.com/pylint-dev/pylint | ||
| rev: v4.0.0 | ||
| hooks: | ||
| - id: pylint |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Clarify pylint configuration approach for consuming repositories.
The pylint hook has no arguments or configuration specified. Based on the PR comments, you mentioned that pylint's scope is unclear and that you would link each repo's pylintrc from the static-checks submodule to the repo root. Please verify this approach will work correctly, as pylint typically requires a configuration file to function properly.
Consider one of the following approaches:
- Add a default pylintrc to this submodule that consuming repos can override
- Document in a README how consuming repos should configure their pylintrc
- Add
args: ['--rcfile=pylintrc']to make the expected configuration file explicit
Run this script to verify how other Python projects in the avocado-framework organization handle pylint configuration:
🏁 Script executed:
#!/bin/bash
# Search for pylint configuration files across avocado-framework repositories
gh repo list avocado-framework --limit 50 --json name,url | \
jq -r '.[] | select(.name | startswith("avocado")) | .name' | \
while read repo; do
echo "=== Checking $repo ==="
gh api "repos/avocado-framework/$repo/contents" --jq '.[] | select(.name | test("pylintrc|.pylintrc|pyproject.toml|setup.cfg")) | .name'
doneLength of output: 398
Provide a default pylintrc and explicit rcfile arg for the pylint hook
The submodule currently lacks a pylintrc (only the main avocado repo has one), so add a pylintrc here and update the pylint hook to reference it via --rcfile. Alternatively, document in a README how consuming repos should supply or override their own pylintrc.
🤖 Prompt for AI Agents
In .pre-commit-config.yaml around lines 31 to 34, the pylint hook is referencing
the pylint repo but there is no pylintrc in this submodule; add a default
pylintrc file into the submodule (e.g., at .pylintrc) and update the pylint hook
to pass the explicit rcfile argument (--rcfile=.pylintrc) so the hook uses the
local config, or alternatively add a README entry explaining that consuming
repos must provide/override a pylintrc and modify the hook to require an
explicit path via configuration.
Hi @PaulYuuu, so the pylint still makes sense in here because it adds much more checks than black or isort. Unfortunately IMO we can't use the pylint pre-commit hook directly, and we need to add the current static-checks/check-lint script into the pre-commit hooks, because in projects like Avocado or AAutils, we use different pylintrc files for different parts of each repo and the check-lint script helps us with that. |
OK, thank you for the explanation, it makes senses for me as I can see aautils has the config so I have an idea that still using the pylint hook, but we can modify the entry as pre-commit support this, like I will try if this can meet our requirement, but this request the avocado-static-check be a submodule should correct the name in each repo(currently aautils uses static-check, vt uses avocado-static-check), or just link the check-lint script from submodule to repo root. |
I see, and I think we can update the submodule name in each repo, it shouldn't be a problem. |
2 participants
Summary by CodeRabbit