Add check for trailing whitespace #13
Conversation
WalkthroughAdds a new GitHub Actions workflow "File formatting check", removes the previous Prettier workflow, updates .gitignore patterns, and makes a one-character formatting change in a helper script heredoc. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant GH as GitHub
participant GA as Actions Runner
participant PR as Pull Request
Dev->>GH: Push / Open PR
GH->>GA: Trigger general_file_formatting_check
GA->>GA: Checkout repo
GA->>GA: Check for filenames with spaces/newlines/tabs
GA->>GA: Scan targeted files for trailing whitespace
GA->>GA: Run Prettier v3.6.0 --check on specified patterns
alt Issues found (whitespace or Prettier)
GA->>GH: Collect lists of offending files
GA->>PR: Post comment with file lists and fix commands
GA->>GH: Exit non-zero (fail)
else No issues
GA->>GH: Exit zero (pass)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
🎨 Code Formatting RequiredTrailing Whitespace IssuesSome files have trailing whitespace (spaces/tabs at end of lines). Files with trailing whitespace:
To fix trailing whitespace: find . -name '*.py' -o -name '*.yml' -o -name '*.yaml' -o -name '*.json' -o -name '*.md' -o -name '*.txt' -o -name '*.sh' -o -name '*.conf' | xargs sed -i 's/[[:space:]]*$//'After fixing:
|
![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 (6)
.gitignore (1)
1-3: Consider also ignoring common .env patternsIf the intent is to ignore local environment files, most projects use “.env” and “.env.” (dot-prefixed). Your current patterns target non-hidden “env.”. If you meant to ignore dot-env files too, add these:
env.* vars.* *.debug +/.env +/.env.*.github/workflows/general_file_formatting_check.yml (5)
33-36: Tighten whitespace scan to avoid false positives and speed upGrep currently recurses into all directories and may traverse irrelevant paths (e.g., .git, node_modules) if present. Excluding them reduces noise and runtime. Also add “-I” to skip binaries.
- WHITESPACE_FILES=$(grep -r --include="*.py" --include="*.yml" --include="*.yaml" --include="*.json" --include="*.md" --include="*.txt" --include="*.sh" --include="*.conf" -l '[[:space:]]$' . 2>/dev/null || true) + WHITESPACE_FILES=$(grep -rI \ + --exclude-dir=".git" --exclude-dir="node_modules" --exclude-dir="dist" --exclude-dir="build" \ + --include="*.py" --include="*.yml" --include="*.yaml" --include="*.json" --include="*.md" --include="*.txt" --include="*.sh" --include="*.conf" \ + -l '[[:space:]]$' . 2>/dev/null || true)
70-73: Make PR commenting resilient for forked PRsFor PRs from forks, GITHUB_TOKEN is read-only; commenting will fail and can break the workflow unnecessarily. Add continue-on-error to the comment step (or handle errors in JS) so formatting failures remain the only blocker.
- - name: Comment on PR (on failure) + - name: Comment on PR (on failure) + continue-on-error: true if: github.event_name == 'pull_request' && (steps.prettier_check.outputs.prettier_passed == 'false' || steps.whitespace_check.outputs.whitespace_found == 'true') uses: actions/github-script@v7
99-101: Provide a cross-platform trailing whitespace fix commandThe current sed -i syntax fails on macOS (BSD sed). Suggest a portable alternative using git ls-files + perl. This reduces accidental edits to untracked files and works on Linux/macOS.
- comment += `find . -name '*.py' -o -name '*.yml' -o -name '*.yaml' -o -name '*.json' -o -name '*.md' -o -name '*.txt' -o -name '*.sh' -o -name '*.conf' | xargs sed -i 's/[[:space:]]*$//'\n`; + comment += `git ls-files -z '*.py' '*.yml' '*.yaml' '*.json' '*.md' '*.txt' '*.sh' '*.conf' | xargs -0 perl -i -pe 's/[ \\t]+$//'\n`;
12-13: Optional: Rename job to reflect broader scopeThe job id “prettier” now performs more than Prettier. Consider renaming to “formatting” or similar.
- prettier: + formatting:Note: Update any external references if you rely on the job name.
60-69: Minor: Simplify listing files needing formattingPrettier’s --list-different already implies a check; you can drop --check. Functionality is unchanged.
- FILES_NEEDING_FORMAT=$(npx prettier@${{ env.PRETTIER_VERSION }} --check ${{ env.PRETTIER_FILES }} --ignore-unknown --list-different 2>/dev/null || true) + FILES_NEEDING_FORMAT=$(npx prettier@${{ env.PRETTIER_VERSION }} --list-different ${{ env.PRETTIER_FILES }} --ignore-unknown 2>/dev/null || true)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.github/workflows/general_file_formatting_check.yml(1 hunks).github/workflows/prettier.yml(0 hunks).gitignore(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/prettier.yml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/general_file_formatting_check.yml
[error] 41-41: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
.gitignore (1)
1-3: LGTM on new ignore patternsThese ignores are reasonable and non-invasive. No functional impact on the repo.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/general_file_formatting_check.yml (1)
41-41: Remove trailing whitespace on this blank lineThis will otherwise trip your own whitespace check and YAML linter.
- +
🧹 Nitpick comments (5)
.github/workflows/general_file_formatting_check.yml (5)
12-13: Consider renaming the job id from "prettier" to "formatting"The job now does both Prettier and trailing whitespace checks; a more generic id improves clarity in logs and future maintenance.
-jobs: - prettier: +jobs: + formatting:
31-48: Harden whitespace check and fix shellcheck warnings (SC2129, SC2086)
- Group writes to $GITHUB_OUTPUT (SC2129).
- Quote $GITHUB_OUTPUT (and other expansions) to avoid word-splitting (SC2086).
- Ignore binary files and common large dirs to speed up and avoid false positives.
- WHITESPACE_FILES=$(grep -r --include="*.py" --include="*.yml" --include="*.yaml" --include="*.json" --include="*.md" --include="*.txt" --include="*.sh" --include="*.conf" -l '[[:space:]]$' . 2>/dev/null || true) + WHITESPACE_FILES="$(grep -rI \ + --exclude-dir='.git' --exclude-dir='node_modules' --exclude-dir='dist' --exclude-dir='build' \ + --include='*.py' --include='*.yml' --include='*.yaml' --include='*.json' \ + --include='*.md' --include='*.txt' --include='*.sh' --include='*.conf' \ + -l '[[:space:]]$' . 2>/dev/null || true)" - if [ -n "$WHITESPACE_FILES" ]; then - echo "whitespace_files<<EOF" >> $GITHUB_OUTPUT - echo "$WHITESPACE_FILES" >> $GITHUB_OUTPUT - echo "EOF" >> $GITHUB_OUTPUT + if [ -n "$WHITESPACE_FILES" ]; then + { + echo "whitespace_files<<EOF" + echo "$WHITESPACE_FILES" + echo "EOF" + } >> "$GITHUB_OUTPUT" echo "❌ Found trailing whitespace in files:" echo "$WHITESPACE_FILES" - echo "whitespace_found=true" >> $GITHUB_OUTPUT + echo "whitespace_found=true" >> "$GITHUB_OUTPUT" else echo "✅ No trailing whitespace found" - echo "whitespace_found=false" >> $GITHUB_OUTPUT + echo "whitespace_found=false" >> "$GITHUB_OUTPUT" fi
50-59: Silence SC2086 by passing patterns as an array to PrettierPrevents unintended word-splitting/globbing while keeping behavior identical.
- # Format JSON, YAML (including workflow files), and Markdown files - if npx prettier@${{ env.PRETTIER_VERSION }} --check ${{ env.PRETTIER_FILES }} --ignore-unknown; then - echo "prettier_passed=true" >> $GITHUB_OUTPUT + # Format JSON, YAML (including workflow files), and Markdown files + # Split env patterns into an array to avoid SC2086 issues + read -r -a PATTERNS <<< "${{ env.PRETTIER_FILES }}" + if npx prettier@${{ env.PRETTIER_VERSION }} --check "${PATTERNS[@]}" --ignore-unknown; then + echo "prettier_passed=true" >> "$GITHUB_OUTPUT" else - echo "prettier_passed=false" >> $GITHUB_OUTPUT + echo "prettier_passed=false" >> "$GITHUB_OUTPUT" fi
60-69: Use array patterns and group output writes (SC2129, SC2086)Mirrors the approach from the Prettier check step and quiets shellcheck.
- # Get list of files that need formatting - FILES_NEEDING_FORMAT=$(npx prettier@${{ env.PRETTIER_VERSION }} --check ${{ env.PRETTIER_FILES }} --ignore-unknown --list-different 2>/dev/null || true) - echo "prettier_files<<EOF" >> $GITHUB_OUTPUT - echo "$FILES_NEEDING_FORMAT" >> $GITHUB_OUTPUT - echo "EOF" >> $GITHUB_OUTPUT + # Get list of files that need formatting + read -r -a PATTERNS <<< "${{ env.PRETTIER_FILES }}" + FILES_NEEDING_FORMAT="$(npx prettier@${{ env.PRETTIER_VERSION }} --check "${PATTERNS[@]}" --ignore-unknown --list-different 2>/dev/null || true)" + { + echo "prettier_files<<EOF" + echo "$FILES_NEEDING_FORMAT" + echo "EOF" + } >> "$GITHUB_OUTPUT"
99-101: Make the fix script robust for filenames with spaces/newlinesUse -print0/-0 and restrict to files to avoid breaking on odd filenames.
- comment += `find . -name '*.py' -o -name '*.yml' -o -name '*.yaml' -o -name '*.json' -o -name '*.md' -o -name '*.txt' -o -name '*.sh' -o -name '*.conf' | xargs sed -i 's/[[:space:]]*$//'\n`; + comment += `find . \\( -name '*.py' -o -name '*.yml' -o -name '*.yaml' -o -name '*.json' -o -name '*.md' -o -name '*.txt' -o -name '*.sh' -o -name '*.conf' \\) -type f -print0 | xargs -0 sed -i 's/[[:space:]]*$//'\n`;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.github/workflows/general_file_formatting_check.yml(1 hunks)dashboard-deployments/system-monitoring-influxdb2-flux-grafana/helper_scripts/generate-env.sh(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- dashboard-deployments/system-monitoring-influxdb2-flux-grafana/helper_scripts/generate-env.sh
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-10T15:15:33.003Z
Learnt from: daniel-falk
PR: fixedit-ai/fixedit-data-agent-examples#5
File: project-strobe-color-from-github-workflow/README.md:126-127
Timestamp: 2025-08-10T15:15:33.003Z
Learning: In the FixedIT Data Agent Extra env configuration, spaces in values are acceptable without quotes. For the GITHUB_WORKFLOW variable in project-strobe-color-from-github-workflow, using a placeholder with spaces like "Your Workflow Name" is preferred as it demonstrates proper configuration for the common case where GitHub workflow names contain spaces.
Applied to files:
.github/workflows/general_file_formatting_check.yml
🪛 actionlint (1.7.7)
.github/workflows/general_file_formatting_check.yml
32-32: shellcheck reported issue in this script: SC2129:style:6:3: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
32-32: shellcheck reported issue in this script: SC2086:info:6:35: Double quote to prevent globbing and word splitting
(shellcheck)
32-32: shellcheck reported issue in this script: SC2086:info:7:31: Double quote to prevent globbing and word splitting
(shellcheck)
32-32: shellcheck reported issue in this script: SC2086:info:8:17: Double quote to prevent globbing and word splitting
(shellcheck)
32-32: shellcheck reported issue in this script: SC2086:info:12:35: Double quote to prevent globbing and word splitting
(shellcheck)
32-32: shellcheck reported issue in this script: SC2086:info:15:36: Double quote to prevent globbing and word splitting
(shellcheck)
52-52: shellcheck reported issue in this script: SC2086:info:3:34: Double quote to prevent globbing and word splitting
(shellcheck)
52-52: shellcheck reported issue in this script: SC2086:info:5:35: Double quote to prevent globbing and word splitting
(shellcheck)
63-63: shellcheck reported issue in this script: SC2129:style:3:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
63-63: shellcheck reported issue in this script: SC2086:info:3:31: Double quote to prevent globbing and word splitting
(shellcheck)
63-63: shellcheck reported issue in this script: SC2086:info:4:33: Double quote to prevent globbing and word splitting
(shellcheck)
63-63: shellcheck reported issue in this script: SC2086:info:5:15: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (3)
.github/workflows/general_file_formatting_check.yml (3)
1-2: Good workflow name and scopeClear name and multi-event trigger look good. Also aligns with your prior preference for human-readable names with spaces.
70-114: PR commenting flow looks solidGood UX: clear sections, actionable commands, and gated to PRs only when issues exist.
115-120: Failure gate is correctJob fails when either Prettier or whitespace check fails. Clear message is helpful.
daniel-falk
force-pushed
the
more-linting
branch
from
a3f36fb to
bdadcb9
Compare
|
Tested OK in #14 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
.github/workflows/general_file_formatting_check.yml (6)
33-40: Beware of false positives in Markdown (hard line breaks)The grep rule will flag intentional trailing double-spaces used for hard line breaks in Markdown. This can create noisy failures.
If this repo uses MD hard line breaks, consider:
- Option A (simple): exclude .md from whitespace scanning and rely on Prettier to format MD, accepting hard breaks.
- Option B (stricter): keep .md, but ignore exactly-two-space hard breaks while still flagging tabs or 3+ spaces at EOL. For example, handle MD separately with a stricter pattern, or switch to a small awk/perl that filters out exactly-two-space endings.
I can provide a precise, portable snippet that handles .md separately if you want to keep .md in scope without breaking valid hard breaks.
35-38: Dotfiles may be missed by grep includesYour includes don’t explicitly match dotfiles like .prettierrc.yml, .yamllint.yml, etc. GNU grep’s --include uses globbing semantics that often skip dotfiles unless you specify them explicitly.
Consider adding explicit dotfile includes:
- --include="*.py" --include="*.yml" --include="*.yaml" --include="*.json" --include="*.md" --include="*.txt" --include="*.sh" --include="*.conf" \ + --include="*.py" --include="*.yml" --include="*.yaml" --include="*.json" --include="*.md" --include="*.txt" --include="*.sh" --include="*.conf" \ + --include=".*.yml" --include=".*.yaml" --include=".*.json" --include=".*.md" \
41-52: Consolidate writes to GITHUB_OUTPUT and quote paths (ShellCheck SC2129/SC2086)Group writes to $GITHUB_OUTPUT and quote it to satisfy ShellCheck and avoid edge cases.
Apply this diff:
- if [ -n "$WHITESPACE_FILES" ]; then - echo "whitespace_files<<EOF" >> $GITHUB_OUTPUT - echo "$WHITESPACE_FILES" >> $GITHUB_OUTPUT - echo "EOF" >> $GITHUB_OUTPUT - - echo "❌ Found trailing whitespace in files:" - echo "$WHITESPACE_FILES" - echo "whitespace_found=true" >> $GITHUB_OUTPUT - else - echo "✅ No trailing whitespace found" - echo "whitespace_found=false" >> $GITHUB_OUTPUT - fi + if [ -n "$WHITESPACE_FILES" ]; then + { + echo "whitespace_files<<EOF" + printf '%s\n' "$WHITESPACE_FILES" + echo "EOF" + echo "whitespace_found=true" + } >> "$GITHUB_OUTPUT" + + echo "❌ Found trailing whitespace in files:" + printf '%s\n' "$WHITESPACE_FILES" + else + echo "✅ No trailing whitespace found" + echo "whitespace_found=false" >> "$GITHUB_OUTPUT" + fi
58-68: Reduce repeated redirects and quote GITHUB_OUTPUT (SC2129/SC2086)Same suggestion here: group output writes and quote the file path.
- if npx prettier@${{ env.PRETTIER_VERSION }} --check ${{ env.PRETTIER_FILES }} --ignore-unknown; then - echo "prettier_passed=true" >> $GITHUB_OUTPUT - echo "prettier_files=" >> $GITHUB_OUTPUT + if npx prettier@${{ env.PRETTIER_VERSION }} --check ${{ env.PRETTIER_FILES }} --ignore-unknown; then + { + echo "prettier_passed=true" + echo "prettier_files=" + } >> "$GITHUB_OUTPUT" else - echo "prettier_passed=false" >> $GITHUB_OUTPUT + echo "prettier_passed=false" >> "$GITHUB_OUTPUT" # Get list of files that need formatting - FILES_NEEDING_FORMAT=$(npx prettier@${{ env.PRETTIER_VERSION }} --list-different ${{ env.PRETTIER_FILES }} --ignore-unknown 2>/dev/null || true) - echo "prettier_files<<EOF" >> $GITHUB_OUTPUT - echo "$FILES_NEEDING_FORMAT" >> $GITHUB_OUTPUT - echo "EOF" >> $GITHUB_OUTPUT + FILES_NEEDING_FORMAT="$(npx prettier@${{ env.PRETTIER_VERSION }} --list-different ${{ env.PRETTIER_FILES }} --ignore-unknown 2>/dev/null || true)" + { + echo "prettier_files<<EOF" + printf '%s\n' "$FILES_NEEDING_FORMAT" + echo "EOF" + } >> "$GITHUB_OUTPUT" fiNote on SC2086 about quoting ${{ env.PRETTIER_FILES }}: it’s currently deliberate so the three patterns expand as separate args (they’re already quoted inside the env var). If you want to silence ShellCheck completely, we can switch to a multiline env var and a Bash array; happy to provide that refactor.
15-20: Optional: make PRETTIER_FILES easier to maintain and ShellCheck-cleanThe current “quoted patterns inside a single env var” trick works, but it’s brittle and trips SC2086. Consider a multiline env var + Bash array for clarity.
Here’s an example of what that could look like (illustrative; not a direct diff):
env: PRETTIER_FILES: | **/*.{json,yml,yaml,md} .github/**/*.{json,yml,yaml,md} .*.{yml,yaml,json,md} - name: Run prettier check shell: bash run: | mapfile -t PATTERNS < <(printf '%s\n' "${PRETTIER_FILES}") if npx prettier@"${{ env.PRETTIER_VERSION }}" --check "${PATTERNS[@]}" --ignore-unknown; then { echo "prettier_passed=true" echo "prettier_files=" } >> "$GITHUB_OUTPUT" else echo "prettier_passed=false" >> "$GITHUB_OUTPUT" FILES_NEEDING_FORMAT="$(npx prettier@"${{ env.PRETTIER_VERSION }}" --list-different "${PATTERNS[@]}" --ignore-unknown 2>/dev/null || true)" { echo "prettier_files<<EOF" printf '%s\n' "$FILES_NEEDING_FORMAT" echo "EOF" } >> "$GITHUB_OUTPUT" fiAlso applies to: 58-59
70-116: PR comment robustness and sizeMinor: extremely long file lists can make the comment noisy or hit limits. Consider truncating the list and attaching the full list as a workflow artifact, or summarizing counts and pointing to the step logs. Not blocking.
I can draft an actions/upload-artifact step that attaches the full lists if useful.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.github/workflows/general_file_formatting_check.yml(1 hunks).github/workflows/prettier.yml(0 hunks).gitignore(1 hunks)dashboard-deployments/system-monitoring-influxdb2-flux-grafana/helper_scripts/generate-env.sh(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/prettier.yml
✅ Files skipped from review due to trivial changes (1)
- dashboard-deployments/system-monitoring-influxdb2-flux-grafana/helper_scripts/generate-env.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-10T15:15:33.003Z
Learnt from: daniel-falk
PR: fixedit-ai/fixedit-data-agent-examples#5
File: project-strobe-color-from-github-workflow/README.md:126-127
Timestamp: 2025-08-10T15:15:33.003Z
Learning: In the FixedIT Data Agent Extra env configuration, spaces in values are acceptable without quotes. For the GITHUB_WORKFLOW variable in project-strobe-color-from-github-workflow, using a placeholder with spaces like "Your Workflow Name" is preferred as it demonstrates proper configuration for the common case where GitHub workflow names contain spaces.
Applied to files:
.github/workflows/general_file_formatting_check.yml
🪛 actionlint (1.7.7)
.github/workflows/general_file_formatting_check.yml
32-32: shellcheck reported issue in this script: SC2129:style:10:3: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
32-32: shellcheck reported issue in this script: SC2086:info:10:35: Double quote to prevent globbing and word splitting
(shellcheck)
32-32: shellcheck reported issue in this script: SC2086:info:11:31: Double quote to prevent globbing and word splitting
(shellcheck)
32-32: shellcheck reported issue in this script: SC2086:info:12:17: Double quote to prevent globbing and word splitting
(shellcheck)
32-32: shellcheck reported issue in this script: SC2086:info:16:35: Double quote to prevent globbing and word splitting
(shellcheck)
32-32: shellcheck reported issue in this script: SC2086:info:19:36: Double quote to prevent globbing and word splitting
(shellcheck)
56-56: shellcheck reported issue in this script: SC2086:info:3:34: Double quote to prevent globbing and word splitting
(shellcheck)
56-56: shellcheck reported issue in this script: SC2086:info:4:29: Double quote to prevent globbing and word splitting
(shellcheck)
56-56: shellcheck reported issue in this script: SC2086:info:6:35: Double quote to prevent globbing and word splitting
(shellcheck)
56-56: shellcheck reported issue in this script: SC2129:style:9:3: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
56-56: shellcheck reported issue in this script: SC2086:info:9:33: Double quote to prevent globbing and word splitting
(shellcheck)
56-56: shellcheck reported issue in this script: SC2086:info:10:35: Double quote to prevent globbing and word splitting
(shellcheck)
56-56: shellcheck reported issue in this script: SC2086:info:11:17: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (1)
.github/workflows/general_file_formatting_check.yml (1)
1-6: Solid, focused workflow rename and scope expansionThe workflow cleanly consolidates Prettier checks and trailing whitespace detection, and the triggers/permissions align with the intent to comment on PRs.
daniel-falk
force-pushed
the
more-linting
branch
from
bdadcb9 to
b92bade
Compare
|
tested OK again in #15 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.github/workflows/general_file_formatting_check.yml (4)
30-45: Harden filename check: exclude heavy dirs and clarify files vs. dirsCurrently, the find runs across the entire tree (including .git) and reports both files and directories, while the message mentions only "files". To make the step faster and clearer, prune heavy dirs and update messaging.
Apply:
- - name: Check for files with spaces in names + - name: Check for files/directories with spaces in names @@ - # If we find files with spaces, etc., we exit directly with an error. That way, - # the rest of the workflow does not need to be compatible with spaces in the - # file names which makes things easier. + # If we find files or directories with spaces, tabs, or newlines, exit with an error. + # This avoids having to make later steps robust to problematic pathnames. @@ - echo "🔍 Checking for files with spaces or newlines in names..." - BAD_FILENAMES=$(find . -name "* *" -o -name $'*\n*' -o -name $'*\t*' 2>/dev/null || true) + echo "🔍 Checking for files or directories with spaces/tabs/newlines in names..." + BAD_FILENAMES=$(find . \ + -path ./.git -prune -o \ + -path ./node_modules -prune -o \ + -path ./dist -prune -o \ + -path ./build -prune -o \ + \( -name "* *" -o -name $'*\n*' -o -name $'*\t*' \) -print 2>/dev/null || true) @@ - echo "❌ Found files with spaces/tabs/newlines in names:" + echo "❌ Found files/directories with spaces/tabs/newlines in names:" @@ - echo "Please rename these files to use underscores or hyphens instead of spaces." + echo "Please rename these paths to use underscores or hyphens instead of spaces."
59-66: Quiet ShellCheck SC2129 and quote $GITHUB_OUTPUTGroup redirects and quote $GITHUB_OUTPUT to satisfy shellcheck and marginally reduce I/O.
- echo "whitespace_files<<EOF" >> $GITHUB_OUTPUT - echo "$WHITESPACE_FILES" >> $GITHUB_OUTPUT - echo "EOF" >> $GITHUB_OUTPUT + { + echo "whitespace_files<<EOF" + echo "$WHITESPACE_FILES" + echo "EOF" + } >> "$GITHUB_OUTPUT" @@ - echo "whitespace_found=true" >> $GITHUB_OUTPUT + echo "whitespace_found=true" >> "$GITHUB_OUTPUT"Also mirror-quote "$GITHUB_OUTPUT" for other echos in this step.
75-85: About SC2086 on PRETTIER_FILES: safe as-is due to pre-expanded quotes, but an array is even cleareractionlint/shellcheck flags SC2086 here. In this workflow, ${{ env.PRETTIER_FILES }} is pre-expanded by the runner into three quoted tokens, so it’s safe. If you want to silence the linter and improve readability, use a small array.
- if npx prettier@${{ env.PRETTIER_VERSION }} --check ${{ env.PRETTIER_FILES }} --ignore-unknown; then + # Split patterns into an array for clarity (avoids SC2086 warnings) + PATTERNS=( "**/*.{json,yml,yaml,md}" ".github/**/*.{json,yml,yaml,md}" ".*.{yml,yaml,json,md}" ) + if npx prettier@${{ env.PRETTIER_VERSION }} --check "${PATTERNS[@]}" --ignore-unknown; then @@ - FILES_NEEDING_FORMAT=$(npx prettier@${{ env.PRETTIER_VERSION }} --list-different ${{ env.PRETTIER_FILES }} --ignore-unknown 2>/dev/null || true) + FILES_NEEDING_FORMAT=$(npx prettier@${{ env.PRETTIER_VERSION }} --list-different "${PATTERNS[@]}" --ignore-unknown 2>/dev/null || true)Note: if you adopt this, you can simplify env.PRETTIER_FILES accordingly or remove it.
87-129: Optional: include a safe, cross-platform one-liner to strip trailing whitespaceThe PR comment currently asks users to fix whitespace manually. Adding a portable snippet can save time and avoids pitfalls with filenames containing spaces/newlines.
- comment += `**To fix:** Remove the trailing spaces/tabs at the end of lines in your favorite editor.\n\n`; + comment += `**To fix:** Remove the trailing spaces/tabs at the end of lines in your favorite editor, or run the following portable command (Linux/macOS):\n\n`; + comment += '```bash\n'; + comment += "find . -type f \\( -name '*.py' -o -name '*.yml' -o -name '*.yaml' -o -name '*.json' -o -name '*.md' -o -name '*.txt' -o -name '*.sh' -o -name '*.conf' \\) \\\n"; + comment += " -not -path './.git/*' -not -path './node_modules/*' -not -path './dist/*' -not -path './build/*' -print0 \\\n"; + comment += " | xargs -0 sed -i.bak -E 's/[[:space:]]+$//' && find . -name '*.bak' -delete\n"; + comment += '```\n\n';This mirrors the exclusions you already use and works on both GNU and BSD sed. It also handles filenames with spaces/newlines safely.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.github/workflows/general_file_formatting_check.yml(1 hunks).github/workflows/prettier.yml(0 hunks).gitignore(1 hunks)dashboard-deployments/system-monitoring-influxdb2-flux-grafana/helper_scripts/generate-env.sh(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/prettier.yml
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- dashboard-deployments/system-monitoring-influxdb2-flux-grafana/helper_scripts/generate-env.sh
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-10T15:15:33.003Z
Learnt from: daniel-falk
PR: fixedit-ai/fixedit-data-agent-examples#5
File: project-strobe-color-from-github-workflow/README.md:126-127
Timestamp: 2025-08-10T15:15:33.003Z
Learning: In the FixedIT Data Agent Extra env configuration, spaces in values are acceptable without quotes. For the GITHUB_WORKFLOW variable in project-strobe-color-from-github-workflow, using a placeholder with spaces like "Your Workflow Name" is preferred as it demonstrates proper configuration for the common case where GitHub workflow names contain spaces.
Applied to files:
.github/workflows/general_file_formatting_check.yml
📚 Learning: 2025-08-07T15:23:05.927Z
Learnt from: daniel-falk
PR: fixedit-ai/fixedit-data-agent-examples#5
File: project-strobe-color-from-github-workflow/README.md:57-62
Timestamp: 2025-08-07T15:23:05.927Z
Learning: In the fixedit-ai/fixedit-data-agent-examples repository, when GitHub generates anchors for headings that contain backticks with spaces around them (like `### `filename` - Description`), it removes the backticks but preserves those spaces as dashes in the anchor, creating triple dashes (---) between the filename and description parts. TOC generators may not handle this correctly and need manual adjustment.
Applied to files:
.github/workflows/general_file_formatting_check.yml
🪛 actionlint (1.7.7)
.github/workflows/general_file_formatting_check.yml
49-49: shellcheck reported issue in this script: SC2129:style:10:3: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
49-49: shellcheck reported issue in this script: SC2086:info:10:35: Double quote to prevent globbing and word splitting
(shellcheck)
49-49: shellcheck reported issue in this script: SC2086:info:11:31: Double quote to prevent globbing and word splitting
(shellcheck)
49-49: shellcheck reported issue in this script: SC2086:info:12:17: Double quote to prevent globbing and word splitting
(shellcheck)
49-49: shellcheck reported issue in this script: SC2086:info:16:35: Double quote to prevent globbing and word splitting
(shellcheck)
49-49: shellcheck reported issue in this script: SC2086:info:19:36: Double quote to prevent globbing and word splitting
(shellcheck)
73-73: shellcheck reported issue in this script: SC2086:info:3:34: Double quote to prevent globbing and word splitting
(shellcheck)
73-73: shellcheck reported issue in this script: SC2086:info:4:29: Double quote to prevent globbing and word splitting
(shellcheck)
73-73: shellcheck reported issue in this script: SC2086:info:6:35: Double quote to prevent globbing and word splitting
(shellcheck)
73-73: shellcheck reported issue in this script: SC2129:style:9:3: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
73-73: shellcheck reported issue in this script: SC2086:info:9:33: Double quote to prevent globbing and word splitting
(shellcheck)
73-73: shellcheck reported issue in this script: SC2086:info:10:35: Double quote to prevent globbing and word splitting
(shellcheck)
73-73: shellcheck reported issue in this script: SC2086:info:11:17: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (3)
.github/workflows/general_file_formatting_check.yml (3)
1-21: Overall workflow structure and intent LGTMGood job renaming and generalizing the workflow, pinning Prettier, and setting minimal permissions. The steps align with the PR objective and are easy to follow.
52-56: Consider scanning only tracked files to reduce noise and speed up grepThe current grep walks the working tree and includes untracked files. If you want to enforce formatting only on tracked content (common in CI), use git ls-files to constrain the search.
Would you like to scope the trailing whitespace scan to tracked files? If yes, replace the grep with:
WHITESPACE_FILES=$(git ls-files -z -- \ '*.py' '*.yml' '*.yaml' '*.json' '*.md' '*.txt' '*.sh' '*.conf' \ | xargs -0 grep -lE '[[:space:]]$' 2>/dev/null || true)This requires git to be available (it is on ubuntu-latest).
131-135: Fail-fast condition is correctClear, deterministic failure when either Prettier or whitespace checks fail. Good job tying it together.
This PR adds a check in the CICD job for trailing spaces. It also renames the file and workflow since the old name is not true anymore now when we do more than just running 'prettier'.
Summary by CodeRabbit