Conversation
| }); | ||
| } | ||
|
|
||
| private escapeQuotes(arg: string): string { |
There was a problem hiding this comment.
New functions that escape (only) quotes or add quotes, are a red flag for me. There are special rules for quoting that differ between shells/platforms.
I'm not sure if it's suitable for your purposes, but some of those rules appear to be encoded into buildShellCommandLine (which appears to be used for launching build tasks).
Taken at face value, a helper function named "quoteArg" is likely inherently problematic. Does it deal with existing quoted or escaped characters properly? Should it be named/commented to enforce the rule that it only be called on strings we generate that are known not to have escaping already (i.e. user input, or parsed from an existing command line). Does it deal with Windows weird rules for quote escaping?
There was a problem hiding this comment.
@Colengms
escapeQuotes will need to stay because of how the macOS launcher works (see line 133), but I can use buildShellCommandLine for building the command line instead of quoteArgs (line 127-128) if that is better. Since I don't have the "original" command, do I just pass empty string?
Looking at the one usage of buildShellCommandLine in the source code, it appears that originalCommand and command should be swapped...?
Fixes: #1201
Supports both the Play button in the editor (run active file) and configurations specified in launch.json. macOS support needs to be tested - I don't currently have access to a machine.
Co-authored by GitHub Copilot