feat: add --print-only flag for shell integrations#4
Open
dimensi wants to merge 5 commits into
Open
Conversation
Add parseArgs helper to strip --print-only from args before dispatch. Gate the [System] startup print on !printOnly so stdout stays clean when --print-only is used. Route API key and config errors to stderr with os.Exit(1) in print-only mode instead of log.Fatal.
- Add TestMain to integration_test.go that builds the nlsh binary before tests run, ensuring TestPrintOnlyNoStartupOutput never hits a missing binary - Extract fatalError(printOnly, format, args) helper to deduplicate the repeated if/printOnly stderr+exit / log.Fatalf pattern across backend setup - Refactor consecutive if/!printOnly + if/printOnly blocks in main() into a single clean if/else structure
Add printOnly bool parameter to handleSingleCommand. In print-only mode, errors are routed to stderr and the bare command is printed to stdout with no ANSI codes or UI chrome, then returns without executing. Add TestPrintOnlyCleanStdout integration test to verify clean stdout output.
📝 WalkthroughWalkthroughnlsh now supports a print-only mode via a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
README.md (1)
97-108: Consider adding a bash equivalent for broader compatibility.The zsh widget is well-written, but many users use bash. A bash equivalent using
bind -xwould help those users.📝 Optional: Add bash equivalent
**Bash equivalent** (add to `~/.bashrc`): ```bash nlsh-readline() { local cmd cmd="$(nlsh --print-only "$READLINE_LINE")" || return [[ -n "$cmd" ]] || return READLINE_LINE="$cmd" READLINE_POINT=${`#READLINE_LINE`} } bind -x '"\C-g": nlsh-readline'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 97 - 108, Add a bash equivalent snippet to the README so bash users can use nlsh via readline: create a function named nlsh-readline that calls nlsh --print-only with "$READLINE_LINE", checks the result is non-empty, then assigns it back to READLINE_LINE and sets READLINE_POINT to ${`#READLINE_LINE`}; register it with bind -x '"\C-g": nlsh-readline' and include this example under the existing zsh widget section so readers of ~/.bashrc can copy it.integration_test.go (2)
59-76: Test validates the contract but relies on implicit failure path.The test correctly validates that stdout remains clean of
[System], ANSI codes, and UI chrome. However, it relies on the binary failing early due to missing API credentials.Consider adding a comment clarifying this dependency, or adding an explicit assertion on the exit code to make the test's assumptions clearer:
📝 Suggested: Make exit code expectation explicit
func TestPrintOnlyCleanStdout(t *testing.T) { cmd := exec.Command("./nlsh", "--print-only", "list files") var stdout, stderr bytes.Buffer cmd.Stdout = &stdout cmd.Stderr = &stderr - // We expect non-zero exit (no API key in test env), but stdout must be clean - cmd.Run() + // Without API credentials, the binary exits early with non-zero status. + // This is expected; the key assertion is that stdout remains clean. + err := cmd.Run() + if err == nil { + t.Log("Note: command succeeded (API key may be set); stdout should still be clean") + } out := stdout.String()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration_test.go` around lines 59 - 76, The test TestPrintOnlyCleanStdout depends on the binary failing (no API key) but doesn't assert that; update the test to capture the result of cmd.Run() (replace the bare cmd.Run() call with err := cmd.Run()), assert that err is non-nil and that the exit code is non-zero (inspect the error as *exec.ExitError and check ExitCode()) to make the failure path explicit, and add a short comment above TestPrintOnlyCleanStdout documenting this dependency; refer to the TestPrintOnlyCleanStdout function and the cmd.Run() invocation when making the change.
17-28: Consider adding error handling for cleanup and usingt.TempDir()pattern.Two minor observations:
os.Remove("nlsh")error is ignored. While typically fine, logging the error in verbose mode could help debug CI issues.Building the binary in the working directory could conflict with parallel test runs or leave artifacts if
m.Run()panics. Consider using a temp directory.These are minor since the current approach works for this use case.
♻️ Optional: More robust cleanup
func TestMain(m *testing.M) { // Build binary for integration tests - build := exec.Command("go", "build", "-o", "nlsh", ".") - build.Dir = "." + binPath := "./nlsh_test_bin" + build := exec.Command("go", "build", "-o", binPath, ".") if out, err := build.CombinedOutput(); err != nil { fmt.Fprintf(os.Stderr, "Failed to build nlsh: %v\n%s\n", err, out) os.Exit(1) } code := m.Run() - os.Remove("nlsh") + if err := os.Remove(binPath); err != nil { + fmt.Fprintf(os.Stderr, "Warning: failed to cleanup test binary: %v\n", err) + } os.Exit(code) }Note: If you use a different binary name, update
TestPrintOnlyCleanStdoutto use the same path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration_test.go` around lines 17 - 28, TestMain currently builds "nlsh" in the repo root, ignores os.Remove errors, and may leave artifacts if m.Run panics; modify TestMain to create a temporary directory (os.MkdirTemp) for the build output, set exec.Command output path and build.Dir to that temp dir, and defer a cleanup function that removes the built binary and logs any removal error (use t.Log or fmt.Fprintf to os.Stderr depending on context); ensure the defer runs before calling m.Run so cleanup always executes, and update any tests that reference the binary path (e.g., TestPrintOnlyCleanStdout) to use the new temp binary path variable.main.go (1)
55-64: Consider supporting--print-only=valueand-pshort flag.The current implementation only matches the exact
--print-onlystring. Users might expect--print-only=trueor a short-pflag to work. Also, arguments like--print-only-somethingwon't be filtered, which is fine, but--print-onlyextrawithout space won't be either (correctly so).This is acceptable for a first iteration, but consider using a flag parsing library (e.g.,
flagpackage orpflag) for consistency if you plan to add more flags.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.go` around lines 55 - 64, The parseArgs function only matches the exact "--print-only" token; update it to also accept the short flag "-p" and "--print-only=value" forms by checking for the exact tokens "--print-only" and "-p", and for a prefix "--print-only=" (use strings.HasPrefix to detect and parse the value) — set printOnly=true when either form is present (treat "-p" or a "--print-only=" with any non-empty value as true) and only append to remaining when the arg is neither of those forms; keep the function name parseArgs and the same return signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@integration_test.go`:
- Around line 59-76: The test TestPrintOnlyCleanStdout depends on the binary
failing (no API key) but doesn't assert that; update the test to capture the
result of cmd.Run() (replace the bare cmd.Run() call with err := cmd.Run()),
assert that err is non-nil and that the exit code is non-zero (inspect the error
as *exec.ExitError and check ExitCode()) to make the failure path explicit, and
add a short comment above TestPrintOnlyCleanStdout documenting this dependency;
refer to the TestPrintOnlyCleanStdout function and the cmd.Run() invocation when
making the change.
- Around line 17-28: TestMain currently builds "nlsh" in the repo root, ignores
os.Remove errors, and may leave artifacts if m.Run panics; modify TestMain to
create a temporary directory (os.MkdirTemp) for the build output, set
exec.Command output path and build.Dir to that temp dir, and defer a cleanup
function that removes the built binary and logs any removal error (use t.Log or
fmt.Fprintf to os.Stderr depending on context); ensure the defer runs before
calling m.Run so cleanup always executes, and update any tests that reference
the binary path (e.g., TestPrintOnlyCleanStdout) to use the new temp binary path
variable.
In `@main.go`:
- Around line 55-64: The parseArgs function only matches the exact
"--print-only" token; update it to also accept the short flag "-p" and
"--print-only=value" forms by checking for the exact tokens "--print-only" and
"-p", and for a prefix "--print-only=" (use strings.HasPrefix to detect and
parse the value) — set printOnly=true when either form is present (treat "-p" or
a "--print-only=" with any non-empty value as true) and only append to remaining
when the arg is neither of those forms; keep the function name parseArgs and the
same return signature.
In `@README.md`:
- Around line 97-108: Add a bash equivalent snippet to the README so bash users
can use nlsh via readline: create a function named nlsh-readline that calls nlsh
--print-only with "$READLINE_LINE", checks the result is non-empty, then assigns
it back to READLINE_LINE and sets READLINE_POINT to ${`#READLINE_LINE`}; register
it with bind -x '"\C-g": nlsh-readline' and include this example under the
existing zsh widget section so readers of ~/.bashrc can copy it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2b28d815-8498-4a94-994e-858a58bf7655
📒 Files selected for processing (3)
README.mdintegration_test.gomain.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--print-onlyflag that outputs only the bare shell command to stdout, with no ANSI colors, no UI chrome, and no executioncmd="$(nlsh --print-only "...")"Usage
Zsh widget example (bind to
Ctrl+G):Test Plan
TestPrintOnlyCleanStdout— verifies stdout has no[System], no ANSI codes, noCommand:/Execute?pkg/tests passcmd="$(nlsh --print-only "list go files")"captures cleanlySummary by CodeRabbit
New Features
Documentation