fix: shell-args-parsing-bug#10637
Conversation
There was a problem hiding this comment.
Code Review
This pull request extracts the shell script argument normalization logic into a reusable helper function normalizeShellScriptArgs in standalone/runtime.js, updates the shell script execution in standalone/runtime.js and standalone/firepit.js to use it, and adds corresponding unit tests. The feedback suggests refactoring normalizeShellScriptArgs to handle nullish arguments defensively and to return early when -c is not found, reducing unnecessary nesting in accordance with the repository style guide.
| exports.normalizeShellScriptArgs = function(args) { | ||
| args = [...args]; | ||
|
|
||
| const index = args.indexOf("-c"); | ||
| if (index !== -1) { | ||
| args.splice(index, 1); | ||
|
|
||
| if (args[index] === "--") { | ||
| args.splice(index, 1); | ||
| } | ||
| } | ||
|
|
||
| return args; | ||
| }; |
There was a problem hiding this comment.
To adhere to the repository style guide regarding reducing nesting and handling edge cases early, we can return early if index === -1. This avoids wrapping the rest of the logic in an outer if block. Additionally, we should defensively handle cases where args might be nullish.
| exports.normalizeShellScriptArgs = function(args) { | |
| args = [...args]; | |
| const index = args.indexOf("-c"); | |
| if (index !== -1) { | |
| args.splice(index, 1); | |
| if (args[index] === "--") { | |
| args.splice(index, 1); | |
| } | |
| } | |
| return args; | |
| }; | |
| exports.normalizeShellScriptArgs = function(args) { | |
| args = [...(args || [])]; | |
| const index = args.indexOf("-c"); | |
| if (index === -1) { | |
| return args; | |
| } | |
| args.splice(index, 1); | |
| if (args[index] === "--") { | |
| args.splice(index, 1); | |
| } | |
| return args; | |
| }; |
References
- Reduce nesting as much as possible. Code should avoid unnecessarily deep nesting or long periods of nesting. Handle edge cases early and exit or fold them into the general case. (link)
Fixes #6446