feat: add tcsh support #17
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds tcsh support and a tcsh test harness; refactors generator.sh to use per-shell inline templates for escaping and control flow, replaces awk color parsing with while/read loops, consolidates inline generation for Rainbow/Reset blocks, and appends nested-color test outputs to existing shell test scripts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant Gen as generator.sh
participant FS as Filesystem
participant Shells as Target Shells (sh/bash/zsh/ksh/fish/tcsh)
participant TestRunner as test-scripts/*
Dev->>Gen: run generator.sh
Gen->>FS: read color table files
FS-->>Gen: return color lines
Gen->>Gen: parse with while/read -> colorCode, labels
Gen->>Gen: select per-shell templates (escape/if/startSym/endSym/para)
loop generate per target shell
Gen->>FS: write ColorEcho.<shell> using inline templates
Gen->>FS: append Rainbow/Reset conditionals using shell template
end
Note right of Shells: tcsh uses alias-based handlers and single-line conditionals
Dev->>TestRunner: run test-scripts/<shell>
TestRunner->>FS: (for tcsh) generate runtime tcsh script mapping aliases
TestRunner->>Shells: pipe or invoke shell to execute tests
Shells->>FS: source ColorEcho.<shell> and run echo tests (including nested-color output)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
Summary of ChangesHello @moormaster, 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 significantly enhances the Highlights
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
|
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||||||||||||
PR Code Suggestions ✨Latest suggestions up to 58d242d
Previous suggestions✅ Suggestions up to commit 6820bcd
|
||||||||||||||||||||||||||||||
![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 adds support for tcsh by refactoring the generator.sh script. The refactoring significantly improves the generator's performance by replacing inefficient grep and awk calls in a loop with a single cat | while read construct, which is a great improvement. However, the changes introduce a critical bug in the echo.Reset function for all shells due to an incorrect tr command, and a sed syntax error in echo.Reset specifically for tcsh. Additionally, the generated code for echo.Rainbow has become less readable for shells that support multi-line functions. I've provided comments with suggestions to fix these issues.
| cat << LOLCAT >> "${tempDist}" | ||
| ${fnName}${startSym} | ||
| ${ifCond} | ||
| echo "\$${para}" | lolcat | ||
| else | ||
| echo "\$${para}" | ||
| ${endIf} | ||
| ${endSym} | ||
| ${fnName}${startSym}${if}${ifCond}${then}echo "${para}" | lolcat${else}echo "${para}"${endIf}${endSym} | ||
| LOLCAT |
There was a problem hiding this comment.
Condensing the if-then-else-fi logic into a single line for echo.Rainbow reduces readability in the generated scripts for shells like bash, zsh, fish, and ksh. While this is necessary for tcsh aliases, it would be better to handle tcsh as a special case and retain the more readable multi-line format for other shells.
You could achieve this with something like:
if [ "${shell}" = "tcsh" ]; then
cat << LOLCAT >> "${tempDist}"
${fnName}${startSym}${if}${ifCond}${then}echo "${para}" | lolcat${else}echo "${para}"${endIf}${endSym}
LOLCAT
else
# The old multi-line cat << LOLCAT block
cat << LOLCAT >> "${tempDist}"
${fnName}${startSym}
${if}${ifCond}${then}
echo "${para}" | lolcat
${else}
echo "${para}"
${endIf}
${endSym}
LOLCAT
fiA similar change should be applied to echo.Reset generation for consistency and readability.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
generator.sh (1)
1-244: Address ShellCheck suggestions and apply shfmt to generator.sh
- Distribution files are up-to-date; no unstaged changes under dist/.
- ShellCheck reports only info-level warnings (SC1091, SC1003, SC2016); adjust quoting/escaping per the suggestions.
- Install/configure shfmt and reformat generator.sh with
shfmt -sr -i 2 -cito enforce consistency.
🧹 Nitpick comments (3)
test-scripts/tcsh (2)
5-5: Add existence check for ColorEcho.tcsh.The script should verify that the ColorEcho.tcsh file exists before attempting to source it, to provide a clearer error message if the file is missing.
Apply this diff:
dir="$(cd "$(dirname "$0")" && pwd)" script="${dir}/../dist/ColorEcho.tcsh" + +if [ ! -f "${script}" ]; then + echo "Error: ${script} not found. Please run generator.sh first." >&2 + exit 1 +fi
8-13: Consider adding test output for better feedback.The test silently executes each alias without providing feedback on success or failure. Consider adding output to confirm which aliases were tested.
{ echo "source \"${script}\"" + echo "echo 'Testing ColorEcho aliases for tcsh...'" awk '/^alias +echo/ {print $2}' "${script}" | while IFS= read -r x; do - echo "${x} \"${x}\"" + echo "echo 'Testing: ${x}'" + echo "${x} \"${x}\"" done + echo "echo 'All tests completed.'" } | tcshgenerator.sh (1)
165-165: Consider removing useless use of cat.The current pattern
cat "${table}" | while read -rcan be simplified towhile read -r ... done < "${table}"to avoid the unnecessary cat process.Apply this diff:
- cat "${table}" | while read -r color colorCode; do + while read -r color colorCode; do # light or not for light in "" "Light"; do if [ "${light}" = "" ]; then code=3 else code=9 fi # style like bold, italic, underline, blinking, strikethrough for style in "" "Bold" "I" "UL" "BL" "ST"; do case "${style}" in "Bold") styleCode='1;' ;; "I") styleCode='3;' ;; "UL") styleCode='4;' ;; "BL") styleCode='5;' ;; "ST") styleCode='9;' ;; "" | *) styleCode= ;; esac for style2 in "" "Bold" "I" "UL" "BL" "ST"; do if [ "${style}" != "${style2}" ]; then case "${style2}" in "Bold") finalStyleCode="${styleCode}1;" ;; "I") finalStyleCode="${styleCode}3;" ;; "UL") finalStyleCode="${styleCode}4;" ;; "BL") finalStyleCode="${styleCode}5;" ;; "ST") finalStyleCode="${styleCode}9;" ;; "" | *) finalStyleCode="${styleCode}" ;; esac else style2= finalStyleCode="${styleCode}" fi echoFunction="$(printf "%secho%s%s%s%s%s" "${fn}" "${dot}" "${light}" "${style}" "${style2}" "${color}")" if ! grep -q "${echoFunction}" "${tempDist}"; then { echo "" printf "%s%s" "${echoFunction}" "${brackets}" # write the code down echo "${startSym}${echo} "'"'"${escape}033[${finalStyleCode}${code}${colorCode}m${para}${escape}033[m"'"'"${endSym}" } >> "${tempDist}" fi done done done - done + done < "${table}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
dist/ColorEcho.bashis excluded by!**/dist/**dist/ColorEcho.fishis excluded by!**/dist/**dist/ColorEcho.kshis excluded by!**/dist/**dist/ColorEcho.shis excluded by!**/dist/**dist/ColorEcho.tcshis excluded by!**/dist/**dist/ColorEcho.zshis excluded by!**/dist/**
📒 Files selected for processing (2)
generator.sh(5 hunks)test-scripts/tcsh(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
@(generator.sh|color table.txt)
📄 CodeRabbit inference engine (AGENTS.md)
Regenerate the distribution scripts with ./generator.sh whenever generator.sh or color table.txt is modified; commit the resulting dist/ColorEcho.* updates together
Files:
generator.sh
**/*.{sh,bash,ksh,zsh,fish,md,markdown}
📄 CodeRabbit inference engine (AGENTS.md)
Honor 2-space indentation (and .editorconfig defaults) for shell and Markdown files
Files:
generator.sh
**/*.{sh,bash,ksh,zsh,fish}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{sh,bash,ksh,zsh,fish}: Format shell scripts with shfmt using flags -sr -i 2 -ci to align with CI
Run ShellCheck on modified shell scripts
Files:
generator.sh
generator.sh
📄 CodeRabbit inference engine (AGENTS.md)
Keep helper names and files consistent with the generator (echo.<Style>); add new helpers only via generator.sh
Files:
generator.sh
🧠 Learnings (2)
📚 Learning: 2025-09-29T15:56:53.647Z
Learnt from: CR
PR: PeterDaveHello/ColorEchoForShell#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T15:56:53.647Z
Learning: Applies to generator.sh : Keep helper names and files consistent with the generator (echo.<Style><Color>); add new helpers only via generator.sh
Applied to files:
generator.sh
📚 Learning: 2025-09-29T15:56:53.647Z
Learnt from: CR
PR: PeterDaveHello/ColorEchoForShell#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T15:56:53.647Z
Learning: Applies to @(generator.sh|color table.txt) : Regenerate the distribution scripts with ./generator.sh whenever generator.sh or color table.txt is modified; commit the resulting dist/ColorEcho.* updates together
Applied to files:
generator.sh
🔇 Additional comments (8)
test-scripts/tcsh (2)
1-6: LGTM!The shebang, error handling, and path resolution are correct and follow best practices.
1-13: ShellCheck OK; shfmt not available
- ShellCheck reports no issues for test-scripts/tcsh.
shfmtcommand not found: install shfmt and runshfmt -sr -i 2 -ci -d test-scripts/tcshto confirm formatting per guidelines.generator.sh (6)
32-32: LGTM!The addition of tcsh to the shell list is correct.
37-119: LGTM!The per-shell configuration variables (escape, if, then, else, endIf, startSym, endSym, para) are correctly defined for each shell. The values are consistent with each shell's syntax requirements.
220-222: LGTM!The tcsh-specific conditional check using
whichand tcsh-style redirection (>& /dev/null) is appropriate for tcsh compatibility.
85-102: Manual verification required for tcsh configuration.The sandbox lacks tcsh; please confirm in a native tcsh shell that:
- ANSI escape sequences work with plain
echo- Parameter substitution via
\!*in aliases passes all arguments&&and||operator precedence yields the intended conditional flow
213-230: tcsh Rainbow alias logic is correct The pipe|binds tighter than&&and||in tcsh, so the alias implements the intended fallback. (web.pa.msu.edu)
203-203: Inline template generates correct escape sequences and quoting for bash, ksh, tcsh, and fish; no changes needed.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
dist/ColorEcho.bashis excluded by!**/dist/**dist/ColorEcho.fishis excluded by!**/dist/**dist/ColorEcho.kshis excluded by!**/dist/**dist/ColorEcho.shis excluded by!**/dist/**dist/ColorEcho.tcshis excluded by!**/dist/**dist/ColorEcho.zshis excluded by!**/dist/**
📒 Files selected for processing (1)
generator.sh(5 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
@(generator.sh|color table.txt)
📄 CodeRabbit inference engine (AGENTS.md)
Regenerate the distribution scripts with ./generator.sh whenever generator.sh or color table.txt is modified; commit the resulting dist/ColorEcho.* updates together
Files:
generator.sh
**/*.{sh,bash,ksh,zsh,fish,md,markdown}
📄 CodeRabbit inference engine (AGENTS.md)
Honor 2-space indentation (and .editorconfig defaults) for shell and Markdown files
Files:
generator.sh
**/*.{sh,bash,ksh,zsh,fish}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{sh,bash,ksh,zsh,fish}: Format shell scripts with shfmt using flags -sr -i 2 -ci to align with CI
Run ShellCheck on modified shell scripts
Files:
generator.sh
generator.sh
📄 CodeRabbit inference engine (AGENTS.md)
Keep helper names and files consistent with the generator (echo.<Style>); add new helpers only via generator.sh
Files:
generator.sh
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: PeterDaveHello/ColorEchoForShell#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T15:56:53.647Z
Learning: Use the scripts under test-scripts/ to validate the corresponding shell variant; rerun all variants after generator or color changes
| ifCond='which lolcat >& /dev/null' | ||
| # tcsh uses aliases instead of functions -> replace ', [ and ] with quoted chars | ||
| trCntrl='\\x27\[:cntrl:\]\\x27' | ||
| ;; |
There was a problem hiding this comment.
tcsh Reset alias deletes apostrophes
trCntrl='\\x27\[:cntrl:\]\\x27' expands to tr -d \x27[:cntrl:]\x27, so the generated tcsh alias removes literal ' (0x27) along with control chars. echo.Reset must not drop regular characters from user text. Please emit the tcsh set as just [:cntrl:], which works inside the single-quoted alias without eating apostrophes.
- trCntrl='\\x27\[:cntrl:\]\\x27'
+ trCntrl='[:cntrl:]'🤖 Prompt for AI Agents
In generator.sh around lines 223-226, trCntrl is set to '\\x27\[:cntrl:\]\\x27'
which expands to tr -d \x27[:cntrl:]\x27 and causes tcsh aliases to remove
literal apostrophes from user text; change the emitted tcsh set to just
'[:cntrl:]' so the alias is defined without surrounding \x27 quotes (i.e.,
assign trCntrl to the plain character class and ensure the alias generation uses
that value inside the single-quoted tcsh alias so apostrophes are preserved).
There was a problem hiding this comment.
Even worse - tcsh takes \x27 literally and removes \, x, 2 and 7 ... fixed that.
There was a problem hiding this comment.
@moormaster, thanks for catching that additional detail and fixing it! You're absolutely right—tcsh would interpret \x27 as four literal characters to remove (\, x, 2, 7) rather than as a hex escape sequence for the apostrophe character. The corrected version with just [:cntrl:] avoids this issue entirely and ensures only control characters are removed from the output, preserving all regular characters including apostrophes.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
moormaster
force-pushed
the
feat/add-tcsh
branch
from
24089bd to
58d242d
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds tcsh shell support to ColorEcho, implementing color echo functions using aliases instead of function declarations since tcsh doesn't support functions. The changes include refactoring the generator to use condensed one-line declarations across all shells and improving cross-shell character escaping compatibility.
Key changes:
- Add tcsh shell support with alias-based color functions using
\!*parameter syntax - Refactor generator to produce one-line function/alias declarations for all shells
- Update character escaping patterns for better cross-shell compatibility
Reviewed Changes
Copilot reviewed 2 out of 8 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| generator.sh | Add tcsh support and refactor to generate condensed one-line declarations |
| dist/ColorEcho.tcsh | New tcsh implementation with 1270+ color aliases |
| test-scripts/tcsh | Test script to validate tcsh alias functionality |
| dist/ColorEcho.{bash,ksh,sh,zsh,fish} | Updated to use condensed one-line format |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @@ -0,0 +1,13 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
The shebang should use tcsh instead of bash since this is a tcsh test script. Using bash shebang conflicts with the tcsh commands used later in the script.
| #!/usr/bin/env bash | |
| #!/usr/bin/env tcsh |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
This is a bash script generating tcsh commands and piping them into tcsh.
| startSym=' { | ||
| ' | ||
| endSym=' | ||
| }' |
There was a problem hiding this comment.
[nitpick] Multi-line string literals with embedded newlines make the code harder to read and maintain. Consider using explicit \n characters or a more readable approach for defining these symbols.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Using \n will literally store \ and n into the variable which is not the same as stoting a linebreak into it.
|
|
||
| # echo.Reset to remove color code on output | ||
| fnName="${fn}echo${dot}Reset${brackets}" | ||
| cat << RESET >> "${tempDist}" |
There was a problem hiding this comment.
[nitpick] The bash parameter expansion ${escape//\//\/\/} for escaping forward slashes is complex and hard to understand. Consider using a more readable approach or adding a comment explaining this escape logic.
| cat << RESET >> "${tempDist}" | |
| cat << RESET >> "${tempDist}" | |
| # Escape forward slashes in the ${escape} variable for use in the sed pattern below. |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Fixed - removed superfluous escaping.
generator.sh
Outdated
| cat "${table}" | while read -r color colorCode; do | ||
| # light or not | ||
| for light in "" "Light"; do |
There was a problem hiding this comment.
Using cat file | while is an anti-pattern. Use while read -r color colorCode < \"${table}\" or while read -r color colorCode; do ... done < \"${table}\" instead to avoid the unnecessary cat process.
| cat "${table}" | while read -r color colorCode; do | |
| # light or not | |
| for light in "" "Light"; do | |
| while read -r color colorCode; do | |
| # light or not | |
| for light in "" "Light"; do | |
| done < "${table}" |
Copilot uses AI. Check for mistakes.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
| alias echo.Black 'echo "\033[30m\!*\033[m"' | ||
|
|
||
| alias echo.BoldBlack 'echo "\033[1;30m\!*\033[m"' | ||
|
|
||
| alias echo.IBlack 'echo "\033[3;30m\!*\033[m"' | ||
|
|
||
| alias echo.ULBlack 'echo "\033[4;30m\!*\033[m"' | ||
|
|
||
| alias echo.BLBlack 'echo "\033[5;30m\!*\033[m"' | ||
|
|
||
| alias echo.STBlack 'echo "\033[9;30m\!*\033[m"' | ||
|
|
||
| alias echo.BoldIBlack 'echo "\033[1;3;30m\!*\033[m"' | ||
|
|
||
| alias echo.BoldULBlack 'echo "\033[1;4;30m\!*\033[m"' | ||
|
|
||
| alias echo.BoldBLBlack 'echo "\033[1;5;30m\!*\033[m"' |
There was a problem hiding this comment.
The generated ColorEcho.tcsh aliases use the builtin echo to emit \033… sequences, for example alias echo.Black 'echo "\033[30m\!*\033[m"'. Unlike bash/zsh, tcsh’s echo does not interpret backslash escape sequences, so these aliases output the literal text \033[30m instead of colored output. Without switching to an explicit escape-aware command such as /bin/echo -e or printf, all of the newly added tcsh color helpers will fail to add color and simply print the escape codes verbatim.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Doing a echo "\033[31mHello world\033[m" perfectly works in tcsh without -e. Passing -e to echo will lead to -e being printed to the output, too.
There was a problem hiding this comment.
Question:
Doing a echo \033[31mHello world\033[m perfectly works in tcsh without -e. Passing -e to echo will lead to -e being printed to the output, too.
Answer:
The tcsh nuance is valid: tcsh’s built-in echo doesn’t use -e and typically honors octal/ESC sequences as typed; adding -e causes it to print “-e”. To avoid generating incorrect aliases in tcsh-related changes, guide the AI to use printf (portable) or bare echo without -e.
Add to your config:
[pr_code_suggestions]
extra_instructions = """\
Shell alias/scripts guidance:
- When suggesting colorized output in tcsh, do NOT use `echo -e`. Either:
(a) use `printf "\033[31m%s\033[m\n" "text"`; or
(b) use `echo "\033[31m... \033[m"` without `-e`.
- If the shell is tcsh/csh-family, prefer printf-based examples for portability.
- Avoid introducing `-e` flags to echo in shell snippets unless explicitly bash-only.
"""Optionally tighten suggestions to avoid wrong shell flags:
[pr_code_suggestions]
focus_only_on_problems = true
suggestions_score_threshold = 6This steers Qodo Merge to produce tcsh-correct snippets and suppress low-value stylistic shell edits.
Relevant Sources:
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
generator.sh (2)
129-131: mktemp GNU option breaks on macOS/BSD; add portable fallback
--suffixisn’t available on BSD mktemp. Use a cross‑platform pattern.- tempDist="$(mktemp --suffix=ColorEcho)" + if mktemp --version >/dev/null 2>&1; then + tempDist="$(mktemp --suffix=ColorEcho)" + else + # BSD/macOS + tempDist="$(mktemp -t ColorEcho.XXXXXX)" + fi
244-247: Background jobs may hide failures; propagate non‑zero exits
set -edoesn’t apply to background jobs, andwaitalone only returns the last status. Collect and fail fast.- } & + } & pids+=($!) done - -wait +fail=0 +for pid in "${pids[@]}"; do + if ! wait "$pid"; then + fail=1 + fi +done +[ "$fail" -eq 0 ] || { echo.BoldRed "Generation failed"; exit 1; }
♻️ Duplicate comments (3)
generator.sh (3)
235-236: Consider multi‑line body for non‑tcsh for readabilityOne‑line
if/elsehurts readability in dist for bash/zsh/ksh/sh/fish. Keep single‑line only for tcsh.
241-241: sed pattern escaping is dense; add a brief commentClarify why
${escape//\//\/\/}and the CSI regex are used.
223-227: tcsh Reset alias uses wrong tr set; removes literal chars instead of control classUse plain POSIX class inside tcsh’s single-quoted alias. Current
\[:cntrl:\]is taken literally by tr.- # tcsh uses aliases instead of functions - # -> remove ' and replace [ and ] with quoted chars - trCntrl='\[:cntrl:\]' + # tcsh alias is single-quoted; emit bare POSIX class + trCntrl='[:cntrl:]'
🧹 Nitpick comments (2)
generator.sh (2)
219-221: Fix ksh lolcat detection redirection
2> /dev/null >&2sends stdout to stderr. Quiet both streams instead.- ifCond='command -v lolcat 2> /dev/null >&2' + ifCond='command -v lolcat > /dev/null 2>&1'
197-205: Use literal match to avoid regex pitfalls when checking for duplicatesFunction names for non-sh shells contain '.', which
greptreats as regex. Use fixed‑string search.- if ! grep -q "${echoFunction}" "${tempDist}"; then + if ! grep -Fq -- "${echoFunction}" "${tempDist}"; then
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
generator.sh(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
@(generator.sh|color table.txt)
📄 CodeRabbit inference engine (AGENTS.md)
Regenerate the distribution scripts with ./generator.sh whenever generator.sh or color table.txt is modified; commit the resulting dist/ColorEcho.* updates together
Files:
generator.sh
**/*.{sh,bash,ksh,zsh,fish,md,markdown}
📄 CodeRabbit inference engine (AGENTS.md)
Honor 2-space indentation (and .editorconfig defaults) for shell and Markdown files
Files:
generator.sh
**/*.{sh,bash,ksh,zsh,fish}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{sh,bash,ksh,zsh,fish}: Format shell scripts with shfmt using flags -sr -i 2 -ci to align with CI
Run ShellCheck on modified shell scripts
Files:
generator.sh
generator.sh
📄 CodeRabbit inference engine (AGENTS.md)
Keep helper names and files consistent with the generator (echo.<Style>); add new helpers only via generator.sh
Files:
generator.sh
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: PeterDaveHello/ColorEchoForShell#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T15:56:53.647Z
Learning: Use the scripts under test-scripts/ to validate the corresponding shell variant; rerun all variants after generator or color changes
🔇 Additional comments (3)
generator.sh (3)
165-209: Nice: switched to while-read with redirectionThis avoids UUOC and subshell pitfalls. LGTM.
32-52: Per‑shell templates look consistentGood separation of per‑shell symbols and quoting, including tcsh aliasing and fish end markers.
Also applies to: 53-68, 69-84, 85-102, 103-119
238-243: dist files are up-to-date Regenerating with generator.sh produced no uncommitted changes.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
generator.sh(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
@(generator.sh|color table.txt)
📄 CodeRabbit inference engine (AGENTS.md)
Regenerate the distribution scripts with ./generator.sh whenever generator.sh or color table.txt is modified; commit the resulting dist/ColorEcho.* updates together
Files:
generator.sh
**/*.{sh,bash,ksh,zsh,fish,md,markdown}
📄 CodeRabbit inference engine (AGENTS.md)
Honor 2-space indentation (and .editorconfig defaults) for shell and Markdown files
Files:
generator.sh
**/*.{sh,bash,ksh,zsh,fish}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{sh,bash,ksh,zsh,fish}: Format shell scripts with shfmt using flags -sr -i 2 -ci to align with CI
Run ShellCheck on modified shell scripts
Files:
generator.sh
generator.sh
📄 CodeRabbit inference engine (AGENTS.md)
Keep helper names and files consistent with the generator (echo.<Style>); add new helpers only via generator.sh
Files:
generator.sh
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: PeterDaveHello/ColorEchoForShell#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T15:56:53.647Z
Learning: Use the scripts under test-scripts/ to validate the corresponding shell variant; rerun all variants after generator or color changes
🪛 Shellcheck (0.11.0)
generator.sh
[warning] 241-241: exec is referenced but not assigned (for output from commands, use "$(exec ...)" ).
(SC2154)
[warning] 241-241: leftQuote is referenced but not assigned.
(SC2154)
[warning] 241-241: rightQuote is referenced but not assigned.
(SC2154)
| ifCond='which lolcat >& /dev/null' | ||
| # tcsh uses aliases instead of functions | ||
| # -> remove ' and replace [ and ] with quoted chars | ||
| trCntrl='\[:cntrl:\]' | ||
| ;; |
There was a problem hiding this comment.
Fix tcsh Reset alias to use the real POSIX control class
trCntrl='\[:cntrl:\]' makes the generated echo.Reset alias run tr -d \[:cntrl:\], which strips the literal characters \, [, :, c, … instead of control characters. As a result, tcsh users lose regular characters (and control chars remain). Emit the raw class so tcsh aliases match the working behavior of the other shells.
- trCntrl='\[:cntrl:\]'
+ trCntrl='[:cntrl:]'🤖 Prompt for AI Agents
In generator.sh around lines 223 to 227, the tcsh reset alias variable trCntrl
is set to '\[:cntrl:\]' which causes tr to delete the literal backslash and
bracket characters instead of POSIX control characters; change the assignment to
emit the raw POSIX class ([:cntrl:]) so the generated tcsh alias uses tr -d
'[:cntrl:]' and removes control characters as intended.
There was a problem hiding this comment.
As stated earlier tcsh we declare aliases instead of functions for tcsh - the alias value is already enclosed in ' chars to prevent escaping issues so we cannot use ' characters inside of the alias value.
There was a problem hiding this comment.
Question:
As stated earlier tcsh we declare aliases instead of functions for tcsh - the alias value is already enclosed in ' chars to prevent escaping issues so we cannot use ' characters inside of the alias value.
Answer:
The tcsh constraint (“alias value is already in single quotes; cannot include ' inside it”) should be communicated to Qodo Merge so suggestions don’t propose edits that introduce single quotes or change escaping incorrectly.
Add targeted guardrails to the Improve tool:
-
Per-PR (comment):
/improve --pr_code_suggestions.extra_instructions="For any tcsh-related shell code or alias generation: do not introduce single quotes inside alias values (they are already enclosed in single quotes). Prefer POSIX character classes without additional backslashes (e.g., use [:cntrl:] not '[:cntrl:]'). When suggesting tr deletions, ensure tcsh alias content remains valid when wrapped in single quotes." -
Persistent (repo config .pr_agent.toml):
[pr_code_suggestions]
extra_instructions="""
Shell-specific constraints:- tcsh aliases are emitted already single-quoted; do not place additional single quotes inside the alias value.
- Use raw POSIX classes like [:cntrl:] without backslash-escaping the brackets/colons when they will live inside a single-quoted tcsh alias.
- When proposing tr -d patterns for tcsh, validate that the pattern will not be interpreted literally due to over-escaping.
"""
If you want to reduce similar noisy/incorrect shell suggestions generally, you can also raise the suggestion threshold:
[pr_code_suggestions]
suggestions_score_threshold = 6
Note: This is a prompting/constraints issue rather than a platform setting; the adjustment steers the model away from invalid tcsh escaping patterns.
Relevant Sources:
- https://qodo-merge-docs.qodo.ai//tools/improve#extra-instructions-and-best-practices
- https://qodo-merge-docs.qodo.ai//faq/index#q:-how-can-i-get-more-tailored-suggestions
- https://qodo-merge-docs.qodo.ai//core-abilities/self_reflection#appendix---relevant-configuration-options
- https://qodo-merge-docs.qodo.ai//usage-guide/additional_configurations#extra-instructions
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the tcsh-specific variables and how the Reset alias is generated
cd generator.sh
# Extract the tcsh case block showing startSym and endSym
echo "=== tcsh case block showing quoting ==="
sed -n '/case "${shell}" in/,/esac/p' generator.sh | sed -n '/"tcsh")/,/;;/p'
echo -e "\n=== Reset alias generation template ==="
sed -n '/echo.Reset to remove color code/,/RESET/p' generator.sh | tail -n 3
# Check if dist/ColorEcho.tcsh exists and show the actual Reset alias
if [ -f dist/ColorEcho.tcsh ]; then
echo -e "\n=== Actual generated Reset alias in dist/ColorEcho.tcsh ==="
grep -A 2 "alias echo.Reset" dist/ColorEcho.tcsh | head -n 3
fi
Length of output: 1072
@moormaster, thank you for the clarification! You're absolutely right that we can't use nested single quotes in tcsh aliases. However, my original suggestion trCntrl='[:cntrl:]' is actually correct for this constraint.
Looking at the generated output:
alias echo.Reset 'echo "\!*" | tr -d \[:cntrl:\] | sed -E "s/\[((;)?[0-9]{1,3}){0,3}m//g" | xargs'The issue is that \[:cntrl:\] makes tr try to delete the literal characters \, [, :, c, n, t, r, l, ] instead of POSIX control characters.
With trCntrl='[:cntrl:]', the generated alias would be:
alias echo.Reset 'echo "\!*" | tr -d [:cntrl:] | sed -E "s/\[((;)?[0-9]{1,3}){0,3}m//g" | xargs'This is correct because:
- No nested single quotes are used
- Inside the single-quoted alias value,
[:cntrl:]appears as-is - When the alias expands and runs,
tr -d [:cntrl:]correctly interprets it as a POSIX character class - For the other shells, the quotes in
trCntrl="'[:cntrl:]'"protect the brackets from shell expansion
The key insight: we don't need to escape the brackets for tcsh because they're already protected by the alias's outer single quotes. The escaping is what's causing the bug.
generator.sh
Outdated
| ${fnName}${startSym} | ||
| echo "\$${para}" | tr -d '[:cntrl:]' | sed -E "s/\\\\[((;)?[0-9]{1,3}){0,3}m//g" | xargs | ||
| ${endSym} | ||
| ${fnName}${startSym}${exec}echo "${para}" | tr -d ${trCntrl} | sed -E ${leftQuote}s/${escape}[((;)?[0-9]{1,3}){0,3}m//g${rightQuote} | xargs${endSym} |
There was a problem hiding this comment.
Quote the sed expression when generating Reset
${leftQuote} / ${rightQuote} are never set, so the generated scripts end up with sed -E s/\[((;)?[0-9]{1,3}){0,3}m//g. In zsh (default nomatch) this throws zsh: no matches found and aborts echo.Reset; in other shells it risks globbing accidents. Inline the double quotes (or actually assign the quote vars) so the substitution stays a single literal argument.
-${fnName}${startSym}${exec}echo "${para}" | tr -d ${trCntrl} | sed -E ${leftQuote}s/${escape}[((;)?[0-9]{1,3}){0,3}m//g${rightQuote} | xargs${endSym}
+${fnName}${startSym}${exec}echo "${para}" | tr -d ${trCntrl} | sed -E "s/${escape//\//\/\/}[((;)?[0-9]{1,3}){0,3}m//g" | xargs${endSym}🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 241-241: exec is referenced but not assigned (for output from commands, use "$(exec ...)" ).
(SC2154)
[warning] 241-241: leftQuote is referenced but not assigned.
(SC2154)
[warning] 241-241: rightQuote is referenced but not assigned.
(SC2154)
🤖 Prompt for AI Agents
In generator.sh around line 241, the sed expression is emitted without quotes
because ${leftQuote}/${rightQuote} are unset, causing shells like zsh to treat
the pattern as globbing; update the generator to emit the sed argument as a
single quoted string (for example wrap the expression in double quotes when
generating) so the produced command becomes sed -E
"s/\[((;)?[0-9]{1,3}){0,3}m//g" (alternatively, ensure
${leftQuote}/${rightQuote} are properly defined before use); this prevents zsh
nomatch errors and accidental globbing while preserving the literal regex.
There was a problem hiding this comment.
Removed changes that do not belong into this pr
${exec}${leftQuote}${rightQuote}
There was a problem hiding this comment.
@moormaster, excellent! Removing those undefined variables keeps the PR focused on tcsh support and eliminates the shellcheck warnings. The sed expression should now work correctly across all shells without the risk of globbing issues or unset variable references.
🧠 Learnings used
Learnt from: CR
PR: PeterDaveHello/ColorEchoForShell#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T15:56:53.647Z
Learning: Applies to @(generator.sh|color table.txt) : Regenerate the distribution scripts with ./generator.sh whenever generator.sh or color table.txt is modified; commit the resulting dist/ColorEcho.* updates together
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
moormaster
force-pushed
the
feat/add-tcsh
branch
from
45c5fcd to
99150cd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
generator.sh(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
@(generator.sh|color table.txt)
📄 CodeRabbit inference engine (AGENTS.md)
Regenerate the distribution scripts with ./generator.sh whenever generator.sh or color table.txt is modified; commit the resulting dist/ColorEcho.* updates together
Files:
generator.sh
**/*.{sh,bash,ksh,zsh,fish,md,markdown}
📄 CodeRabbit inference engine (AGENTS.md)
Honor 2-space indentation (and .editorconfig defaults) for shell and Markdown files
Files:
generator.sh
**/*.{sh,bash,ksh,zsh,fish}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{sh,bash,ksh,zsh,fish}: Format shell scripts with shfmt using flags -sr -i 2 -ci to align with CI
Run ShellCheck on modified shell scripts
Files:
generator.sh
generator.sh
📄 CodeRabbit inference engine (AGENTS.md)
Keep helper names and files consistent with the generator (echo.<Style>); add new helpers only via generator.sh
Files:
generator.sh
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: PeterDaveHello/ColorEchoForShell#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T15:56:53.647Z
Learning: Use the scripts under test-scripts/ to validate the corresponding shell variant; rerun all variants after generator or color changes
| # rainbow output relys on lolcat | ||
| fnName="${fn}echo${dot}Rainbow${brackets}" | ||
| case "${shell}" in | ||
| "fish") | ||
| ifCond="if command -v lolcat > /dev/null" | ||
| ifCond='command -v lolcat > /dev/null' | ||
| trCntrl="'[:cntrl:]'" | ||
| ;; | ||
| "ksh") | ||
| ifCond='if command -v lolcat 2> /dev/null >&2; then' | ||
| ifCond='command -v lolcat 2> /dev/null >&2' | ||
| trCntrl="'[:cntrl:]'" | ||
| ;; | ||
| "tcsh") | ||
| ifCond='which lolcat >& /dev/null' | ||
| # tcsh uses aliases instead of functions | ||
| # -> remove ' and replace [ and ] with quoted chars | ||
| trCntrl='\[:cntrl:\]' | ||
| ;; |
There was a problem hiding this comment.
tcsh Reset still strips apostrophes.
trCntrl='\[:cntrl:\]' makes the generated tcsh alias run tr -d \[:cntrl:\], which keeps removing literal ' (and other printable chars) instead of only control characters. In tcsh aliases the outer single quotes already protect the brackets, so we can safely emit the raw POSIX class.
Update the tcsh branch to set trCntrl='[:cntrl:]', matching the behavior of the other shells.
🤖 Prompt for AI Agents
In generator.sh around lines 211 to 227, the tcsh branch currently sets trCntrl
to a backslash-escaped class (\[:cntrl:\]) which causes the generated tcsh alias
to emit an escaped bracket form and strip printable characters; change the
assignment to set trCntrl to the POSIX class wrapped in single quotes, i.e.,
replace the backslash-escaped value with a quoted [:cntrl:] so the generated
tcsh alias uses '[:cntrl:]' like the other shells.
There was a problem hiding this comment.
tr is not the problem here. What strippes the ' is the echo "\!*" command at the beginning and the xargscommand at the end:
> alias echo.Reset 'echo "\!*" | tr -d \[:cntrl:\] | sed -E "s/\[((;)?[0-9]{1,3}){0,3}m//g" | xargs'
> echo.Reset "A sentence with '\033[32mgreen\033[m' flavor"
A sentence with green flavor
What works is to replace "\!*" with \!* and replace | xargs with ; echo:
> alias echo.Reset 'echo \!* | tr -d \[:cntrl:\] | sed -E "s/\[((;)?[0-9]{1,3}){0,3}m//g"; echo'
> echo.Reset "A sentence with '\033[32mgreen\033[m' flavor"
A sentence with 'green' flavor
There was a problem hiding this comment.
@moormaster, thank you for the correction! You're absolutely right—the issue is with how the parameter expansion is quoted in echo and the use of xargs, not with tr.
The fix requires tcsh-specific handling for the Reset function. Here's the code change needed:
# echo.Reset to remove color code on output
fnName="${fn}echo${dot}Reset${brackets}"
case "${shell}" in
"tcsh")
# tcsh needs unquoted parameter expansion and ; echo instead of | xargs
cat << RESET >> "${tempDist}"
${fnName}${startSym}echo ${para} | tr -d ${trCntrl} | sed -E "s/${escape}[((;)?[0-9]{1,3}){0,3}m//g"; echo${endSym}
RESET
;;
*)
cat << RESET >> "${tempDist}"
${fnName}${startSym}echo "${para}" | tr -d ${trCntrl} | sed -E "s/${escape}[((;)?[0-9]{1,3}){0,3}m//g" | xargs${endSym}
RESET
;;
esacThis change:
- Removes the double quotes around
${para}for tcsh (so\!*remains unquoted) - Replaces
| xargswith; echofor tcsh
The generated tcsh alias will then be:
alias echo.Reset 'echo \!* | tr -d \[:cntrl:\] | sed -E "s/\[((;)?[0-9]{1,3}){0,3}m//g"; echo'This preserves apostrophes as your testing confirmed.
There was a problem hiding this comment.
Question:
tr is not the problem here. What strippes the ' is the echo \!* command at the beginning and the xargscommand at the end: > alias echo.Reset 'echo \!* | tr -d [:cntrl:] | sed -E s/\[((;)?[0-9]{1,3}){0,3}m//g | xargs' > echo.Reset A sentence with \'\033[32mgreen\033[m\' flavor A sentence with green flavor What works is to replace \!* with !* and replace | xargs with ; echo: > alias echo.Reset 'echo !* | tr -d [:cntrl:] | sed -E s/\[((;)?[0-9]{1,3}){0,3}m//g; echo' > echo.Reset A sentence with \'\033[32mgreen\033[m\' flavor A sentence with 'green' flavor
Answer:
It looks like the AI suggestion targeted trCntrl escaping, but the real tcsh issue is the alias expansion and xargs eating quotes. You can steer Qodo Merge’s Improve suggestions away from changing tr and toward the alias fixes by adding targeted extra instructions:
[pr_code_suggestions]
extra_instructions = """\
In shell aliases (tcsh), avoid suggesting changes to POSIX character classes like [:cntrl:].
Focus instead on:
- Replacing 'echo \!*' with 'echo !*' to prevent backslash-escaped history expansion that strips apostrophes.
- Avoid using '... | xargs' when preserving quotes is required; prefer ending with '; echo' to keep literals intact.
When reviewing tcsh reset/echo aliases, ensure suggestions preserve literal single quotes in output.
"""If you also want these guardrails during reviews:
[pr_reviewer]
extra_instructions = """\
For shell (tcsh) alias changes, verify that:
- 'echo !*' is used instead of 'echo \!*' to keep apostrophes.
- 'xargs' is not used when output quoting must be preserved; prefer '; echo'.
Avoid recommending edits that change '[:cntrl:]' semantics when the issue is alias expansion/quoting.
"""This will guide future suggestions/reviews to propose:
- echo.Reset alias using:
echo !* | tr -d [:cntrl:] | sed -E 's/\[((;)?[0-9]{1,3}){0,3}m//g'; echo - No changes to trCntrl escaping unless truly necessary.
Relevant Sources:
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
There was a problem hiding this comment.
Fixed the stripping of ' by echo.Reset and added a test output to all test scripts
There was a problem hiding this comment.
@moormaster, excellent! Thank you for fixing the echo.Reset apostrophe preservation issue and adding the test outputs to all test scripts. The tcsh-specific handling with unquoted parameter expansion and ; echo instead of | xargs is the right approach. 🎉
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
dist/ColorEcho.bashis excluded by!**/dist/**dist/ColorEcho.fishis excluded by!**/dist/**dist/ColorEcho.kshis excluded by!**/dist/**dist/ColorEcho.shis excluded by!**/dist/**dist/ColorEcho.tcshis excluded by!**/dist/**dist/ColorEcho.zshis excluded by!**/dist/**
📒 Files selected for processing (7)
generator.sh(4 hunks)test-scripts/bash(1 hunks)test-scripts/fish(1 hunks)test-scripts/ksh(1 hunks)test-scripts/sh(1 hunks)test-scripts/tcsh(1 hunks)test-scripts/zsh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test-scripts/tcsh
🧰 Additional context used
📓 Path-based instructions (4)
@(generator.sh|color table.txt)
📄 CodeRabbit inference engine (AGENTS.md)
Regenerate the distribution scripts with ./generator.sh whenever generator.sh or color table.txt is modified; commit the resulting dist/ColorEcho.* updates together
Files:
generator.sh
**/*.{sh,bash,ksh,zsh,fish,md,markdown}
📄 CodeRabbit inference engine (AGENTS.md)
Honor 2-space indentation (and .editorconfig defaults) for shell and Markdown files
Files:
generator.sh
**/*.{sh,bash,ksh,zsh,fish}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{sh,bash,ksh,zsh,fish}: Format shell scripts with shfmt using flags -sr -i 2 -ci to align with CI
Run ShellCheck on modified shell scripts
Files:
generator.sh
generator.sh
📄 CodeRabbit inference engine (AGENTS.md)
Keep helper names and files consistent with the generator (echo.<Style>); add new helpers only via generator.sh
Files:
generator.sh
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: PeterDaveHello/ColorEchoForShell#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T15:56:53.647Z
Learning: Use the scripts under test-scripts/ to validate the corresponding shell variant; rerun all variants after generator or color changes
🔇 Additional comments (9)
test-scripts/zsh (1)
12-13: LGTM! Nested color test addition.The new final line tests nested color codes with apostrophes and ANSI escapes, validating that
echo.Resetcorrectly strips color codes while preserving text. This aligns with similar additions across other test scripts.test-scripts/ksh (1)
12-13: LGTM! Consistent nested color test.Identical test pattern to other shells, using ksh-specific function naming (
echoReset,echoRedwithout dot separators).test-scripts/sh (1)
11-12: LGTM! Consistent nested color test.Matches the test pattern added to other shells, using sh-specific function naming.
test-scripts/fish (1)
10-11: LGTM! Consistent nested color test.Follows the same nested color test pattern, using fish-specific function naming with dot separators.
test-scripts/bash (1)
11-12: LGTM! Consistent nested color test.Completes the consistent nested color test pattern across all shell test scripts.
generator.sh (4)
32-32: LGTM! tcsh added to supported shells.The addition of tcsh to the shell loop enables tcsh support throughout the generator.
85-102: LGTM! tcsh-specific configuration.The tcsh configuration correctly handles alias-based function emulation with:
aliasinstead offunction\!*for parameter expansion (tcsh-specific)&&and||operators instead ofif/then/else(tcsh constraint)- Single-quoted alias wrapper
165-209: LGTM! Improved loop pattern.Replacing
cat "${table}" | whilewithwhile read -r color colorCode; do ... done < "${table}"eliminates the unnecessarycatprocess and follows shell best practices.
234-236: One-line format for Rainbow is acceptable.While multi-line formatting would be more readable for non-tcsh shells, the one-line format is necessary to support tcsh's alias constraints (no multi-line conditionals). The trade-off is acceptable given tcsh compatibility requirements.
| # echo.Reset to remove color code on output | ||
| fnName="${fn}echo${dot}Reset${brackets}" | ||
| cat << RESET >> "${tempDist}" | ||
| ${fnName}${startSym} | ||
| echo "\$${para}" | tr -d '[:cntrl:]' | sed -E "s/\\\\[((;)?[0-9]{1,3}){0,3}m//g" | xargs | ||
| ${endSym} | ||
| ${fnName}${startSym}echo ${para} | tr -d ${trCntrl} | sed -E "s/${escape}[((;)?[0-9]{1,3}){0,3}m//g"; echo${endSym} | ||
| RESET |
There was a problem hiding this comment.
CRITICAL: Reset function generation is not shell-specific.
Line 241 generates the same Reset implementation for all shells:
echo ${para} | tr -d ${trCntrl} | sed -E "..."; echoThis is correct for tcsh (uses echo \!* unquoted, ends with ; echo), but breaks non-tcsh shells:
-
Unquoted
${para}: For bash/zsh/ksh/sh,${para}expands to$*, which splits on whitespace when unquoted. Input like"Red and 'green'"will be split into separate arguments. -
; echoinstead of| xargs: The trailing; echoprints a newline but doesn't consolidate the output like| xargsdoes, changing the function's behavior.
The correct approach is to use shell-specific generation like Rainbow does for ifCond:
Apply this diff to generate shell-specific Reset implementations:
- # echo.Reset to remove color code on output
- fnName="${fn}echo${dot}Reset${brackets}"
- cat << RESET >> "${tempDist}"
-${fnName}${startSym}echo ${para} | tr -d ${trCntrl} | sed -E "s/${escape}[((;)?[0-9]{1,3}){0,3}m//g"; echo${endSym}
-RESET
+ # echo.Reset to remove color code on output
+ fnName="${fn}echo${dot}Reset${brackets}"
+ case "${shell}" in
+ "tcsh")
+ # tcsh needs unquoted \!* and ; echo
+ cat << RESET >> "${tempDist}"
+${fnName}${startSym}echo ${para} | tr -d ${trCntrl} | sed -E "s/${escape}[((;)?[0-9]{1,3}){0,3}m//g"; echo${endSym}
+RESET
+ ;;
+ *)
+ # Other shells need quoted "$*" and | xargs
+ cat << RESET >> "${tempDist}"
+${fnName}${startSym}echo "${para}" | tr -d ${trCntrl} | sed -E "s/${escape}[((;)?[0-9]{1,3}){0,3}m//g" | xargs${endSym}
+RESET
+ ;;
+ esac🧩 Analysis chain
Verify Reset function behavior across all shells.
Given the critical issue flagged above, please run the test scripts for all shells to verify that the echo.Reset function works correctly, especially with the new nested color test case that includes apostrophes and spaces.
Run this script to validate Reset behavior across shells:
🏁 Script executed:
#!/bin/bash
# Description: Test echo.Reset function across all shells to verify correct behavior
for shell in bash zsh ksh sh fish tcsh; do
echo "=== Testing $shell ==="
# Check if shell is available
if ! command -v "$shell" &> /dev/null; then
echo " [SKIP] $shell not installed"
continue
fi
# Run the shell-specific test script
test_script="test-scripts/$shell"
if [ -f "$test_script" ]; then
echo " Running $test_script..."
if "$shell" "$test_script" 2>&1 | tail -5; then
echo " [PASS] $shell test completed"
else
echo " [FAIL] $shell test failed"
fi
else
echo " [SKIP] Test script not found"
fi
echo ""
doneLength of output: 850
Fix test harness to correctly detect failures
Using "$shell" "$test_script" | tail -5 masks errors because tail exits zero. Enable set -o pipefail (or capture the test script’s exit code before piping), and explicitly fail when a shell or its test script is missing. Then rerun tests for all installed shells to confirm echo.Reset handles nested colors and apostrophes.
There was a problem hiding this comment.
-
Unquoted
${para}: For bash/zsh/ksh/sh,${para} expands to $ *, which splits on whitespace when unquoted.
This shouldn't be a problem since echo processes all these arguments. -
; echoinstead of| xargs: The trailing;echo prints a newline but doesn't consolidate the output like| xargsdoes, changing the function's behavior.
The change is intended since xargs were causing'to be stripped from the input before outputting them, too.$ echo "'color'" 'color' $ echo "'color'" | xargs color
There was a problem hiding this comment.
Question:
-
Unquoted
${para}: For bash/zsh/ksh/sh,${para} expands to $ *, which splits on whitespace when unquoted. This shouldn't be a problem since echo processes all these arguments. 2.; echoinstead of| xargs: The trailing;echo prints a newline but doesn't consolidate the output like| xargsdoes, changing the function's behavior. The change is intended since xargs were causing'to be stripped from the input before outputting them, too.$ echo \'color\' 'color' $ echo \'color\' | xargs color
Answer:
To avoid similar shell-specific false positives in future AI suggestions, add targeted guidance to the Improve tool so it respects per-shell semantics and quoting rules.
Suggested configuration:
[pr_code_suggestions]
extra_instructions = """\
Shell scripts:
- Do not propose a single generic implementation across different shells when behavior differs (bash/zsh/ksh/sh vs tcsh/fish).
- When modifying functions that expand $* / $@, preserve the project’s established quoting and argument-joining behavior. Do not replace ; echo with | xargs unless the repo explicitly standardizes on xargs behavior.
- Respect tcsh-specific patterns (e.g., unquoted \!* and trailing '; echo') where used; for POSIX shells prefer explicit "$*" or "$@" only if the current codebase standard requires it.
- When suggesting cross-shell changes, propose a case/switch per-shell block instead of a one-size-fits-all command.
- Include a brief note explaining any change in argument splitting, quoting, or newline handling.
- Avoid suggestions that could strip quotes (e.g., via xargs) unless accompanied by a safe, tested alternative.
"""Optional, to reduce noise on shell code:
- Increase the suggestion threshold for shell files:
[pr_code_suggestions] suggestions_score_threshold = 6
- Ignore shell test harness lines like tail masking exit codes by being explicit in prompts (keeps focus on PR’s intended behavior):
[pr_code_suggestions] extra_instructions = """\ When reviewing shell test harnesses, ensure failure detection is robust (e.g., set -o pipefail) and don’t introduce constructs that mask non‑zero exits."""
If you want stricter safeguarding for shell changes:
- Require per-shell rationale:
[pr_code_suggestions] extra_instructions = """\ For any change to shell commands, add a one-line rationale per targeted shell (bash/zsh/ksh/sh, tcsh, fish) and state whether quoting/whitespace semantics change."""
Relevant Sources:
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify xargs quote-stripping behavior and test Reset function across shells
echo "=== Testing xargs quote behavior ==="
echo "Test 1: Simple quoted string"
echo "'color'" | xargs
echo ""
echo "Test 2: String with apostrophes and spaces"
echo "Red and 'green'" | xargs
echo ""
echo "Test 3: String with ANSI codes and quotes"
echo -e "\033[31mRed and 'green'\033[m" | xargs
echo ""
echo "=== Comparing old vs new Reset implementation ==="
echo "Old (with xargs):"
echo -e "\033[31mRed and 'green'\033[m" | tr -d '[:cntrl:]' | sed -E "s/\\[((;)?[0-9]{1,3}){0,3}m//g" | xargs
echo ""
echo "New (with ; echo):"
echo -e "\033[31mRed and 'green'\033[m" | tr -d '[:cntrl:]' | sed -E "s/\\[((;)?[0-9]{1,3}){0,3}m//g"; echo
echo ""
echo "=== Testing word splitting with unquoted variable ==="
para="Red and 'green'"
echo "Quoted: echo \"\$para\""
echo "$para"
echo ""
echo "Unquoted: echo \$para"
echo $para
echo ""
echo "Both produce same output from echo command perspective"
Length of output: 499
@moormaster, you're absolutely right—thank you for the clarification! My test results confirm your explanation:
1. Unquoted ${para} is safe: While word-splitting occurs, echo simply outputs all arguments space-separated, so the final result is identical whether the variable is quoted or not.
2. Removing xargs is a bug fix: The old implementation was incorrectly stripping quotes from the output:
Old (with xargs): Red and green # ← quotes lost ❌
New (without xargs): Red and 'green' # ← quotes preserved ✅
This change improves the echo.Reset function by preserving the original formatting. I apologize for incorrectly flagging this as a critical issue—it's actually a valid improvement. The implementation works correctly across all shells as-is.
🧬 Code Graph Analysis Results
- File: dist/ColorEcho.zsh, lines 234-236
function echo.BoldRed() {
echo -e "\\033[1;31m$*\\033[m"
}
- File: dist/ColorEcho.bash, lines 234-236
function echo.BoldRed() {
echo -e "\\033[1;31m$*\\033[m"
}
- File: dist/ColorEcho.zsh, lines 442-444
function echo.BoldGreen() {
echo -e "\\033[1;32m$*\\033[m"
}
- File: dist/ColorEcho.bash, lines 442-444
function echo.BoldGreen() {
echo -e "\\033[1;32m$*\\033[m"
}
- File: dist/ColorEcho.zsh, lines 650-652
function echo.BoldYellow() {
echo -e "\\033[1;33m$*\\033[m"
}
- File: dist/ColorEcho.bash, lines 650-652
function echo.BoldYellow() {
echo -e "\\033[1;33m$*\\033[m"
}
🧠 Learnings used
Learnt from: CR
PR: PeterDaveHello/ColorEchoForShell#0
File: AGENTS.md:0-0
Timestamp: 2025-09-29T15:56:53.647Z
Learning: Applies to @(generator.sh|color table.txt) : Regenerate the distribution scripts with ./generator.sh whenever generator.sh or color table.txt is modified; commit the resulting dist/ColorEcho.* updates together
User description
See Issue #2
PR Type
Enhancement
Description
Add tcsh shell support with alias-based color functions
Refactor generator to support one-line function declarations
Update existing shell outputs to use condensed format
Fix character escaping for cross-shell compatibility
Diagram Walkthrough
File Walkthrough
generator.sh
Add tcsh support and refactor generatorgenerator.sh
ColorEcho.tcsh
New tcsh color echo implementationdist/ColorEcho.tcsh
aliasinstead offunctiondeclarations\!*ColorEcho.bash
Refactor to one-line declarationsdist/ColorEcho.bash
trcommandColorEcho.ksh
Refactor to one-line declarationsdist/ColorEcho.ksh
trcommandColorEcho.sh
Refactor to one-line declarationsdist/ColorEcho.sh
trcommandColorEcho.zsh
Refactor to one-line declarationsdist/ColorEcho.zsh
trcommandColorEcho.fish
Refactor to one-line declarationsdist/ColorEcho.fish
trcommandtcsh
Add tcsh testing scripttest-scripts/tcsh
Summary by CodeRabbit