-
Notifications
You must be signed in to change notification settings - Fork 30
Improve shell command formatting #740
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?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,4 +1,5 @@ | ||||||||||||||||||||||||||||||||||
| export function quoteStringIfNecessary(arg: string): string { | ||||||||||||||||||||||||||||||||||
| arg = arg.trim(); | ||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if we really need this. Seems like the only places we call this are: vscode-python-environments/src/features/execution/runAsTask.ts Lines 29 to 35 in e0eedd3
and vscode-python-environments/src/features/execution/runInBackground.ts Lines 10 to 16 in e0eedd3
And the executable string that is passed in seems like its path to Python binary.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here: vscode-python-environments/src/extension.ts Line 492 in e0eedd3
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. looks like vscode-python-environments/src/extension.ts Line 493 in 0390029
|
||||||||||||||||||||||||||||||||||
| if (arg.indexOf(' ') >= 0 && !(arg.startsWith('"') && arg.endsWith('"'))) { | ||||||||||||||||||||||||||||||||||
| return `"${arg}"`; | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
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.
Thanks for the PR.
Looks like command will never be undefined here, we could probably remove the
| undefinedThere 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.
undefinedis required here since theif ... elsetests onselectedOption.labelbelow don't have a defaultelse, therefore the ts compiler will complain withts(2454).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.
Now I'm thinking can we just simplify the second
else if (...Resolve Environment)intoelse?There is only two options that
selectionOption.labelcan take.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.
That's true but I didn't want to introduce any unrelated changes with possibly undesirable effects, keeping the PR context relevant and minimal. Should I still change it?
Uh oh!
There was an error while loading. Please reload this page.
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.
That's fine, we can leave it as else if, in case there is additional label in future. We should probably return early in case command is ever undefined though and log.
If you do this, we won't need
if (command) { ..