fix(tests): use ERE grep -E instead of BRE alternation, fix newline check (t143)#448
fix(tests): use ERE grep -E instead of BRE alternation, fix newline check (t143)#448marcusquinn merged 1 commit intomainfrom
Conversation
…heck (t143) - Replace 9 instances of grep '\|' with grep -E '|' for readability - Fix imprecise embedded-newline check to use python3 JSON key inspection - Extract duplicated shell-script file iteration into list_shell_scripts() Closes #442
Summary of ChangesHello @marcusquinn, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the reliability, readability, and maintainability of the test suite. It standardizes regular expression usage in Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Caution Review failedThe pull request is closed. WalkthroughIntroduces a helper function Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Sat Feb 7 17:58:51 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several good improvements. It standardizes on grep -E for better portability and readability, refactors duplicated script-listing logic into a helper function, and replaces a fragile grep pipeline with a robust Python script for JSON validation. My review includes a couple of suggestions to further refine the new helper function for better performance and to improve the Python script's resource management.
| list_shell_scripts() { | ||
| git -C "$REPO_DIR" ls-files '*.sh' | while read -r f; do echo "$REPO_DIR/$f"; done | ||
| } |
There was a problem hiding this comment.
For improved conciseness and performance, especially with a large number of files, consider using sed to prepend the directory path instead of a while read loop. The shell loop invokes a new echo process for every file, which can be inefficient.
| list_shell_scripts() { | |
| git -C "$REPO_DIR" ls-files '*.sh' | while read -r f; do echo "$REPO_DIR/$f"; done | |
| } | |
| list_shell_scripts() { | |
| git -C "$REPO_DIR" ls-files '*.sh' | sed "s,^,$REPO_DIR/," | |
| } |
| import json, sys | ||
| d = json.load(open('$REPO_DIR/configs/pandoc-config.json.txt')) | ||
| bad = [k for k in d if '\n' in k] | ||
| sys.exit(1 if bad else 0) |
There was a problem hiding this comment.
The embedded Python script opens a file but doesn't explicitly close it. While the operating system will clean this up for a short-lived script, it's a best practice to use a with open(...) statement to ensure resources are properly managed. This makes the code more robust and idiomatic.
| import json, sys | |
| d = json.load(open('$REPO_DIR/configs/pandoc-config.json.txt')) | |
| bad = [k for k in d if '\n' in k] | |
| sys.exit(1 if bad else 0) | |
| import json, sys | |
| with open('$REPO_DIR/configs/pandoc-config.json.txt') as f: | |
| d = json.load(f) | |
| sys.exit(1 if [k for k in d if '\n' in k] else 0) |
…y errors (t147.2) (#451) * chore: mark t142 complete in TODO.md (#449) * chore: mark t135.9 complete in TODO.md (#448) * fix(supervisor): triage PR #392 review feedback - stderr, sort, deploy errors (t147.2) - Add post-PR states to ORDER BY CASE in cmd_status and cmd_list - Add 300s timeout to deploy setup.sh with portable fallback - Deploy failures now transition to 'failed' instead of silently marking 'deployed' - Redirect cmd_pr_lifecycle output to post-pr.log instead of /dev/null - Fix missing $SUPERVISOR_DB arg in db() calls for no_pr retry counter - Remove unused no_pr_key variable Dismissed 3 of 6 review threads as already fixed or invalid: - gh pr merge stderr: already uses 2>&1 - Fallback PR lookup: already uses git -C and gh pr list --repo - Missing pr_review:deployed transition: not needed, complete:deployed handles it
…147.1) (#450) * chore: mark t142 complete in TODO.md (#449) * chore: mark t135.9 complete in TODO.md (#448) * fix(supervisor): add missing $SUPERVISOR_DB arg to db() calls, remove HOMEBREW_PREFIX guard (t147.1) - Add $SUPERVISOR_DB as first arg to both db() calls in no_pr retry counter logic (lines 3165, 3183). Without it, sqlite3 treated the SQL as a filename and failed silently — retry counter never persisted, making the 5-attempt threshold unreachable. - Remove unused no_pr_key variable. - Remove HOMEBREW_PREFIX guard around PATH augmentation. The idempotent PATH check already prevents duplicates; the guard was overly restrictive for cron environments where HOMEBREW_PREFIX may be set without all tool paths present. Triaged all 4 unresolved review threads from PR #435: - 3 fixed (critical db bug, unused var, PATH guard) - 1 acknowledged won't-fix (json_extract counter reset is by design) Refs: GH#438, t147.1
1 participant
Summary
grep '\|'(BRE alternation) withgrep -E '|'(ERE) for improved readability and POSIX portabilitygrep -P '\x0a'pipeline (which couldn't reliably detect newlines in JSON keys) withpython3JSON key inspectiongit ls-files '*.sh'file-iteration logic intolist_shell_scripts()helper functionTesting
bash -nsyntax check: PASSshellcheck: 0 issuesCloses
Closes #442
Summary by CodeRabbit