ci: add pre-commit hooks for code quality#178
ci: add pre-commit hooks for code quality#178bugman-007 wants to merge 2 commits intoLight-Heart-Labs:mainfrom
Conversation
Adds comprehensive pre-commit hooks for automated code quality checks: Hooks added: - shellcheck: Lints shell scripts (excludes SC1091, SC2034, SC2086) - ruff: Python linting (E, F, W checks, ignores E501 line length) - black: Python formatting (line-length=100) - trailing-whitespace, end-of-file-fixer - check-yaml, check-json Also updates CONTRIBUTING.md with pre-commit setup instructions.
|
" |
Lightheartdevs
left a comment
There was a problem hiding this comment.
Review: ⚠️ REQUEST CHANGES (minor)
This is a well-structured PR that makes sensible additions to the pre-commit configuration. One small fix needed.
What works well
- Hook selections are appropriate — shellcheck, ruff, black, trailing-whitespace, end-of-file-fixer, check-yaml, check-json
- All versions pinned — gitleaks v8.21.2, shellcheck v0.10.0.1, ruff v0.3.4, black 24.3.0, pre-commit-hooks v5.0.0
- shellcheck config is sensible —
--severity=erroronly catches real bugs, exclusions (SC1091/SC2034/SC2086) are reasonable for this codebase - ruff scoped correctly —
^dream-server/.*\.py$ check-yaml --unsafeis necessary for docker-compose custom constructors- CONTRIBUTING.md documentation is clear and helpful
Issue: Premature claim in CONTRIBUTING.md
The added text says:
All Python files have been formatted with
black --line-length=100.
This is only true after PR #177 (black formatting baseline) merges. If #178 lands first, this statement is false. Either:
- Option A: Remove this sentence and add it in #177 instead
- Option B: Land #177 first, then rebase #178
The PR description already notes the dependency on #177, so this is likely just an ordering oversight.
Suggestions for follow-up (non-blocking)
- Consider centralizing ruff config in a
pyproject.tomlto keep CI and pre-commit in sync - SC2086 exclusion is understandable for initial adoption but worth tracking for eventual removal
end-of-file-fixerandtrailing-whitespacewill auto-modify files on firstpre-commit run --all-files— expected but could surprise contributors
CI note
All failures are pre-existing on main. Lint Python with Ruff and Lint shell scripts both pass.
Small fix, then good to go. Nice work.
Addresses review feedback on PR Light-Heart-Labs#178. The statement about all Python files being formatted with black is only true after PR Light-Heart-Labs#177 merges. Removed the premature claim to avoid misleading contributors
AddressedRemoved the premature claim about black formatting being complete. The statement "All Python files have been formatted with The pre-commit hooks documentation remains intact - they will enforce black formatting going forward once #177 lands. Changes: Removed 2 lines from CONTRIBUTING.md (commit 31b1918) Kindly review this update, thanks. |
|
Closing — draft pre-commit hooks PR, depends on #177 which is also being closed. Please rebase and re-open when ready. |
Adds comprehensive pre-commit hooks for automated code quality checks:
Hooks added:
Also updates CONTRIBUTING.md with pre-commit setup instructions.