✨(firmware_mod): Add pre-commit hooks for code quality and formatting#1914
✨(firmware_mod): Add pre-commit hooks for code quality and formatting#1914mdeweerd wants to merge 9 commits intoEliasKotlyar:betafrom
Conversation
mdeweerd
commented
Dec 17, 2025
mdeweerd
commented
- Add pre-commit hooks for code quality and formatting
- Include hooks for Markdown, YAML, JSON, shell scripts, and text files
- Configure hooks for various tools like mdformat, pre-commit-hooks, prettier, yamllint, codespell, beautysh, and shellcheck
- Set up manual stages for some hooks to avoid changing files too much yet
- Exclude specific files and directories from certain hooks
- Apply multiple fixes identified by tools
There was a problem hiding this comment.
Pull request overview
This PR introduces pre-commit hooks infrastructure to enforce code quality and formatting standards across the firmware modification codebase. The changes include configuration files for automated code quality checks and apply numerous fixes identified by various linting tools.
Key changes:
- Added comprehensive pre-commit configuration with hooks for Markdown, YAML, JSON, and shell scripts
- Set up GitHub Actions workflow for automated pre-commit checks on pull requests
- Applied multiple automated fixes including spelling corrections, proper quoting in shell scripts, and security improvements
Reviewed changes
Copilot reviewed 44 out of 50 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
.pre-commit-config.yaml |
Configures pre-commit hooks for mdformat, prettier, yamllint, codespell, beautysh, and shellcheck |
.github/workflows/pre-commit.yml |
Sets up CI workflow to run pre-commit checks automatically |
pyproject.toml |
Adds codespell and yamlfix tool configurations |
.gitignore |
Excludes node_modules generated by tools |
| Shell scripts (multiple) | Adds shellcheck directives, fixes quoting issues, improves variable expansions, and replaces deprecated operators |
| Documentation files | Corrects spelling errors (seperators→separators, trough→through, informations→information, etc.) |
| HTML files | Fixes HTML attribute typos (allign→align) |
| Python files | Corrects shebang positioning |
| Various files | Removes trailing whitespace and ensures proper end-of-file markers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { [ "$send_telegram" = true ] && { [ "$telegram_alert_type" = video+image ] || [ "$telegram_alert_type" = video ]; }; } || | ||
| { [ "$send_matrix" = true ] && { [ "$matrix_alert_type" = video+image ] || [ "$matrix_alert_type" = video ]; }; } || |
There was a problem hiding this comment.
The use of parentheses in the test condition for lines 230-231 creates a complex conditional structure. While the syntax is corrected with proper braces and semicolons, this creates deeply nested conditionals that may be hard to read. Consider extracting these conditions into separate boolean variables for better readability.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 50 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { [ "$send_telegram" = true ] && { [ "$telegram_alert_type" = video+image ] || [ "$telegram_alert_type" = video ]; }; } || | ||
| { [ "$send_matrix" = true ] && { [ "$matrix_alert_type" = video+image ] || [ "$matrix_alert_type" = video ]; }; } || |
There was a problem hiding this comment.
Lines 230-231 use complex nested braces for conditional logic that is hard to read and potentially error-prone. The original code using parentheses with the '-o' operator was clearer. The current transformation changes semantics: the original condition was "(A && (B || C)) || D", but the new version appears to be "{ A && { B || C; }; } || D" which has different shell evaluation behavior.
| { [ "$send_telegram" = true ] && { [ "$telegram_alert_type" = video+image ] || [ "$telegram_alert_type" = video ]; }; } || | |
| { [ "$send_matrix" = true ] && { [ "$matrix_alert_type" = video+image ] || [ "$matrix_alert_type" = video ]; }; } || | |
| { [ "$send_telegram" = true ] && [ "$telegram_alert_type" = "video+image" ]; } || | |
| { [ "$send_telegram" = true ] && [ "$telegram_alert_type" = "video" ]; } || | |
| { [ "$send_matrix" = true ] && [ "$matrix_alert_type" = "video+image" ]; } || | |
| { [ "$send_matrix" = true ] && [ "$matrix_alert_type" = "video" ]; } || |
| { [ "$send_telegram" = true ] && { [ "$telegram_alert_type" = video+image ] || [ "$telegram_alert_type" = video ]; }; } || | ||
| { [ "$send_matrix" = true ] && { [ "$matrix_alert_type" = video+image ] || [ "$matrix_alert_type" = video ]; }; } || |
There was a problem hiding this comment.
The string comparisons on lines 230-231 are missing quotes around the values 'video+image' and 'video', while similar comparisons elsewhere in the file (lines 51, 361, 388) have these values properly quoted. This inconsistency could lead to word splitting issues or unexpected behavior.
| { [ "$send_telegram" = true ] && { [ "$telegram_alert_type" = video+image ] || [ "$telegram_alert_type" = video ]; }; } || | |
| { [ "$send_matrix" = true ] && { [ "$matrix_alert_type" = video+image ] || [ "$matrix_alert_type" = video ]; }; } || | |
| { [ "$send_telegram" = true ] && { [ "$telegram_alert_type" = "video+image" ] || [ "$telegram_alert_type" = "video" ]; }; } || | |
| { [ "$send_matrix" = true ] && { [ "$matrix_alert_type" = "video+image" ] || [ "$matrix_alert_type" = "video" ]; }; } || |
| VER="Need upgrade to have VERSION file" | ||
| fi | ||
| MQTT_COMMAND="/system/sdcard/bin/mosquitto_pub.bin -h "$HOST" -p "$PORT" -u "$USER" -P "$PASS" -t" | ||
| MQTT_COMMAND="/system/sdcard/bin/mosquitto_pub.bin -h \"$HOST\" -p \"$PORT\" -u \"$USER\" -P \"$PASS\" -t" |
There was a problem hiding this comment.
The MQTT_COMMAND variable on line 30 contains quoted variables that will be evaluated when MQTT_COMMAND is expanded. This could lead to unexpected behavior if HOST, PORT, USER, or PASS contain special characters. The previous version without escaped quotes was likely safer. When MQTT_COMMAND is used later with shellcheck disable=2086, the double quoting may cause issues.
|
@mdeweerd can you do the PR against the beta branch please? |
- Add pre-commit hooks for code quality and formatting - Include hooks for Markdown, YAML, JSON, shell scripts, and text files - Configure hooks for various tools like mdformat, pre-commit-hooks, prettier, yamllint, codespell, beautysh, and shellcheck - Set up manual stages for some hooks to avoid changing files too much yet - Exclude specific files and directories from certain hooks - Apply multiple fixes identified by tools
Added shellcheck directives to improve code quality and maintainability. This includes: - Adding source directives for included files - Disabling specific shellcheck warnings where appropriate - Using proper quoting for variables and command substitutions
- Replace 'shellscript source' comments with 'shellcheck source' directives in multiple scripts - Fix potential issues with file path handling in getFonts function - Fix shellcheck notices
- Replace `-o` with `||` for better readability and consistency
- Add shellcheck directives for better code quality
- Improve variable handling with `${var:?}` to prevent errors
- Update conditional statements for better readability
Update the skip pattern in the codespell configuration to ensure consistent formatting. Also fix a typo in the quote_representation parameter.
mdeweerd
force-pushed
the
qual/precommit
branch
from
2a06b43 to
77db520
Compare
- Added error code 2143 to shellcheck args to ignore specific warnings - Fixed variable quoting in merge_config.sh for better shell compatibility - Improved comparison operator in merge_config.sh
Changed comparison operators from '==' to '=' in light sensor conditions to follow shell script best practices. This change improves code consistency and readability without altering functionality.
The motor control command was missing quotes around the expression, which could cause issues with negative values. Added quotes to ensure proper command execution.
|
Changed ;-). |
|
@mdeweerd have you tested functionality on device? |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 50 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if [ "$LIGHT_SENSOR" == 'hw' ] | ||
| then | ||
| # shellcheck disable=2086 | ||
| /system/sdcard/bin/mosquitto_pub.bin -h "$HOST" -p "$PORT" -u "$USER" -P "$PASS" -t "${TOPIC}"/brightness ${MOSQUITTOPUBOPTS} ${MOSQUITTOOPTS} -m "$(ldr status)" | ||
| elif [ $LIGHT_SENSOR == 'virtual' ] | ||
| elif [ "$LIGHT_SENSOR" == 'virtual' ] |
There was a problem hiding this comment.
Inconsistent comparison operator usage. In this file, the double equals operator (==) is still used for string comparison, but in mqtt-status-interval.sh the same comparisons were changed to use the single equals operator (=). For POSIX shell compatibility and consistency across the codebase, these should also be changed to use the single equals operator (=).
