fix!: strip shell extensions from shim target name#57
Open
nadalaba wants to merge 1 commit into
Open
Conversation
PR Summary by Qodofix!: strip shell extensions from target shim name (support shims of shims) WalkthroughsDescription• Strip common shell extensions from the target shim basename by default. • Prevent generating an sh shim with a misleading .ps1/.cmd/.sh extension. • Add an opt-out flag and an e2e regression test for issue #10. Diagramgraph TD
A["Consumer code"] --> B["cmdShim(src,to,opts)"] --> C{"Strip ext?"}
C -- "yes" --> D["Normalize target name"] --> E["cmdShim_()"] --> F["writeAllShims()"] --> G["sh/.cmd/.ps1 shims"]
C -- "no" --> E
High-Level AssessmentThe following are alternative approaches to this PR: 1. Strip only for the sh shim variant
2. Enforce/validate that `to` must be extensionless
3. Conditionally strip only when `to` extension mismatches detected runtime
Recommendation: The chosen approach (strip common shell extensions by default, with an explicit opt-out) is the best tradeoff: it fixes the real-world confusion/bug scenario (sh shim ending in .ps1) without requiring callers to redesign how they compute File ChangesBug fix (1)
Tests (1)
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a new ChangesShell Extension Stripping
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Line 42: Update the README entry for opts.progArgs to fix the grammar: change
the phrase "`will be prepend to any CLI arguments`" to "`will be prepended to
any CLI arguments`" so the description for opts.progArgs reads correctly.
In `@test/e2e.test.js`:
- Line 99: Replace the typo in the inline comment that currently reads "it's s
powershell script" with "it's a powershell script"; locate the comment string
"it's s powershell script" in test/e2e.test.js and update it to the corrected
wording to remove the extra 's'.
- Around line 94-115: The test 'generate shims with {
stripShellExtensionFromShim: true }' assumes Windows behavior (expecting a .cmd
shim) but is not limited to Windows; either wrap this test in describeOnWindows
or make it platform-agnostic by calling cmdShim with createCmdFile: true and
adjusting assertions based on cmdExtension. Locate the test (the async test
calling cmdShim with stripShellExtensionFromShim: true) and update it to use
describeOnWindows(...) or add the option createCmdFile: true and branch the
expected files using cmdExtension so the assertion matches both Windows and
POSIX.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a8cb064b-dbbd-4183-a3f3-c0a128c5cec7
📒 Files selected for processing (3)
README.mdsrc/index.tstest/e2e.test.js
📜 Review details
🔇 Additional comments (4)
src/index.ts (3)
87-92: LGTM!
129-129: LGTM!
163-169: LGTM!README.md (1)
43-43: LGTM!
nadalaba
changed the title
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/e2e.test.js (1)
94-114:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard this regression test for Windows-only behavior (or branch expected files by platform).
Line 113 can still fail off-Windows because
script${cmdExtension}may collapse toscript, creating an impossible expected duplicate. Since this case is explicitly Windows-focused, preferdescribeOnWindows(...)here, or compute expectations conditionally.Suggested minimal fix
-describe('create a shim of a pwsh shim with the same name in another location', () => { +describeOnWindows('create a shim of a pwsh shim with the same name in another location', () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e.test.js` around lines 94 - 114, This Windows-only regression test ("generate shims with { stripShellExtensionFromShim: true }" in test/e2e.test.js) should be guarded or have platform-specific expectations: either wrap the test in describeOnWindows(...) (or skip when process.platform !== 'win32') or compute the expected files array conditionally using process.platform and cmdExtension so you don't compare duplicate names on non-Windows; update the assertion to use the platform-aware expected list instead of the hardcoded ['script', `script${cmdExtension}`, 'script.ps1'].
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@test/e2e.test.js`:
- Around line 94-114: This Windows-only regression test ("generate shims with {
stripShellExtensionFromShim: true }" in test/e2e.test.js) should be guarded or
have platform-specific expectations: either wrap the test in
describeOnWindows(...) (or skip when process.platform !== 'win32') or compute
the expected files array conditionally using process.platform and cmdExtension
so you don't compare duplicate names on non-Windows; update the assertion to use
the platform-aware expected list instead of the hardcoded ['script',
`script${cmdExtension}`, 'script.ps1'].
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dea168d1-e044-42f5-9835-c41a78dbfa5f
📒 Files selected for processing (2)
README.mdtest/e2e.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
nadalaba
changed the title
nadalaba
force-pushed
the
pnpm/shims-of-shims
branch
from
12ecd50 to
e2a4563
Compare
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.
Generating a
shshim for a PowerShell script currently produces a shim namedscript.ps1, which makes PowerShell treat it as a PowerShell script.BREAKING CHANGE: strip common shell extensions from the
target/toname before creating the shims by default. This behavior can be reverted by setting new optionstripShellExtensionFromShim: false.Fix #10
Summary by CodeRabbit
stripShellExtensionFromShimoption (enabled by default) to control whether common shell extensions are removed from shim naming, preventing ambiguous extension combinations.progArgsto reflect its optional nature and improved wording, and documented the newstripShellExtensionFromShimoption.