-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
refactor: cd() avoid arg mutation
#1315
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
antongolub
commented
Aug 11, 2025
- Tests pass
- Appropriate changes to README are included in PR
|
Why this change? |
|
Aligns overload implementations across the code: replaces args mutations with a recursive call. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the cd() function to avoid mutating the input argument by using recursion instead of reassignment when handling ProcessOutput instances.
- Replaces argument mutation with recursive call pattern
- Adds explicit
voidreturn type annotation to the TypeScript version
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/core.ts | Refactors cd() to use recursion instead of mutating the dir parameter |
| build/core.cjs | Compiled JavaScript output reflecting the same refactoring changes |
| if (dir instanceof ProcessOutput) return cd(dir.toString().trim()) | ||
|
|
||
| $.log({ kind: 'cd', dir, verbose: !$.quiet && $.verbose }) | ||
| process.chdir(dir) |
Copilot
AI
Aug 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The recursive call creates unnecessary function call overhead. Consider using a local variable assignment instead: const dirStr = dir instanceof ProcessOutput ? dir.toString().trim() : dir; then proceed with the rest of the function using dirStr.
| if (dir instanceof ProcessOutput) return cd(dir.toString().trim()) | |
| $.log({ kind: 'cd', dir, verbose: !$.quiet && $.verbose }) | |
| process.chdir(dir) | |
| const dirStr = dir instanceof ProcessOutput ? dir.toString().trim() : dir; | |
| $.log({ kind: 'cd', dir: dirStr, verbose: !$.quiet && $.verbose }) | |
| process.chdir(dirStr) |