Skip to content

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

almarouk
Copy link
Contributor

Consolidates shell command formatting logic into a shared utility function that is used for running in terminal and PET command execution, among other places.
This improves consistency and command path handling.

This should also fix PET command execution on Windows using PowerShell (requires &).

Copy link
Contributor

@Copilot Copilot AI left a 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 consolidates shell command formatting logic into a shared utility function to improve consistency across the codebase and fix PowerShell command execution on Windows.

  • Introduces centralized command formatting through getShellCommandAsString function
  • Fixes PowerShell command execution by properly adding & prefix when needed
  • Refactors duplicate shell command formatting logic in terminal and PET execution

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
src/features/terminal/shells/common/shellUtils.ts Updates PowerShell command formatting to add & prefix for non-quoted commands
src/features/terminal/runInTerminal.ts Replaces inline shell formatting logic with centralized utility function
src/features/execution/execUtils.ts Adds string trimming to quote function for better argument handling
src/extension.ts Refactors PET command execution to use centralized shell formatting

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Consolidates shell command formatting logic into a shared utility function that is used for running in terminal and PET command execution, among other places
@@ -435,6 +436,7 @@ export async function activate(context: ExtensionContext): Promise<PythonEnviron
commands.registerCommand('python-envs.runPetInTerminal', async () => {
try {
const petPath = await getNativePythonToolsPath();
let command: { subcommand: string; args?: string[] } | undefined;
Copy link
Contributor

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 | undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

undefined is required here since the if ... else tests on selectedOption.label below don't have a default else, therefore the ts compiler will complain with ts(2454).

Copy link
Contributor

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) into else?
There is only two options that selectionOption.label can take.

Copy link
Contributor Author

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?

command = { subcommand: 'resolve', args: [inputPath.trim()] };
}

if (command) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if (command) { could also go away if we remove undefined from type above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See reply to previous comment.

@@ -1,4 +1,5 @@
export function quoteStringIfNecessary(arg: string): string {
arg = arg.trim();
Copy link
Contributor

Choose a reason for hiding this comment

The 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:

let executable = environment.execInfo?.activatedRun?.executable ?? environment.execInfo?.run.executable;
if (!executable) {
traceWarn('No Python executable found in environment; falling back to "python".');
executable = 'python';
}
// Check and quote the executable path if necessary
executable = quoteStringIfNecessary(executable);

and
let executable = environment.execInfo?.activatedRun?.executable ?? environment.execInfo?.run.executable;
if (!executable) {
traceWarn('No Python executable found in environment; falling back to "python".');
executable = 'python';
}
// Check and quote the executable path if necessary
executable = quoteStringIfNecessary(executable);

And the executable string that is passed in seems like its path to Python binary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here:

terminal.sendText(`"${petPath}" resolve "${inputPath.trim()}"`, true);

inputPath is user-provided and could have leading white spaces. I did keep the .trim() in this case before eventually processing the string with getShellCommandAsString but I thought also of adding it in the quoting logic for consistency and robustness in future use cases. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants