Skip to content

Adopt argsCanBeInterpretedByShell from DAP - #155663

Merged
roblourens merged 1 commit into
mainfrom
roblourens/issue149910
Jul 20, 2022
Merged

Adopt argsCanBeInterpretedByShell from DAP#155663
roblourens merged 1 commit into
mainfrom
roblourens/issue149910

Conversation

@roblourens

Copy link
Copy Markdown
Member

For #149910

if (args.length > 0) {
const cmd = quote(args.shift()!);
const arg = args.shift()!;
const cmd = argsCanBeInterpretedByShell ? arg : quote(arg);

@weinand Andre Weinand (weinand) Jul 20, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to consider to duplicate the argument processing into two branches of an if-statement controlled by the argsCanBeInterpretedByShell flag. Keeping the two cases apart makes them easier to understand than having one piece of code sprinkled with the argsCanBeInterpretedByShell flag (especially if we'll have to add more logic for future fixes...)

}
for (const a of args) {
command += (a === '<' || a === '>') ? a : quote(a);
command += (a === '<' || a === '>' || argsCanBeInterpretedByShell) ? a : quote(a);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here the logic is too simple - no need to separate the logic...

@weinand Andre Weinand (weinand) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently the argsCanBeInterpretedByShell flag is not used for external terminals. We should create a (debt?) issue for investigating this...

@roblourens
roblourens merged commit 60f09d6 into main Jul 20, 2022
@roblourens
roblourens deleted the roblourens/issue149910 branch July 20, 2022 13:02
@github-actions github-actions Bot locked and limited conversation to collaborators Sep 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants