fix: completions on git bash for windows#28122
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds Windows/MSYS path conversion for tar extraction with relaxed symlink error handling on Windows; moves PowerShell completions into the shell switch; normalizes SHELL detection across platforms; exposes a Windows key for the SHELL env var; and adds Windows-targeted regression tests for completions. Changes
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/build/download.ts`:
- Around line 121-143: When only symlink extraction failures are detected
(inside the if block where process.platform === "win32" and fatalLines.length
=== 0), emit a warning before continuing: log a clear message that tar
extraction had symlink-only failures including the tarball identifier (tarball)
and the filtered stderr lines (fatalLines or result.stderr) to aid debugging;
use the project's logger if available (e.g., processLogger.warn) or console.warn
as a fallback, and do not change the existing BuildError throw behavior for real
failures.
In `@test/regression/issue/25042.test.ts`:
- Around line 29-45: In the test case inside the test(...) block that calls
spawnSync (variables stdout, stderr, exitCode), move the assertion that inspects
stdout (the expect on stdoutText.toContain("bun")) to occur before the
expect(exitCode).toBe(0) so the test checks output content prior to exit status;
update the sequence around the lines using stdout.toString() and the two expect
calls accordingly in the same test function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c99367ae-5c2c-495a-966b-e93e64524c65
📒 Files selected for processing (5)
scripts/build/download.tssrc/cli/install_completions_command.zigsrc/cli/shell_completions.zigsrc/env_var.zigtest/regression/issue/25042.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/25042.test.ts`:
- Around line 8-27: The test "does not show PowerShell error when SHELL is bash"
currently only checks non-zero exitCode branch but doesn't assert that exitCode
is one of the expected values; update the assertion to explicitly validate
exitCode is either 0 or 1 (e.g. assert [0,1] contains exitCode or two explicit
expects) using the existing exitCode variable, and keep the existing stderrText
PowerShell checks inside the non-zero branch so unrelated failures don't pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a511aaf3-fa87-4a07-8010-be556793985e
📒 Files selected for processing (2)
scripts/build/download.tstest/regression/issue/25042.test.ts
What does this PR do?
Fixes #25662.
How did you verify your code works?
Proof that the PR works: