Skip to content
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

Added option to run multiple Python files in separate terminals #21223

Merged
merged 8 commits into from
May 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,12 @@
"icon": "$(play)",
"title": "%python.command.python.execInTerminalIcon.title%"
},
{
"category": "Python",
"command": "python.execInNewTerminal",
"icon": "$(play)",
"title": "%python.command.python.execInNewTerminal.title%"
},
{
"category": "Python",
"command": "python.debugInTerminal",
Expand Down Expand Up @@ -1626,6 +1632,13 @@
"title": "%python.command.python.execInTerminalIcon.title%",
"when": "false && editorLangId == python"
},
{
"category": "Python",
"command": "python.execInNewTerminal",
"icon": "$(play)",
"title": "%python.command.python.execInNewTerminal.title%",
"when": "false && editorLangId == python"
},
{
"category": "Python",
"command": "python.debugInTerminal",
Expand Down Expand Up @@ -1784,6 +1797,12 @@
"title": "%python.command.python.execInTerminalIcon.title%",
"when": "resourceLangId == python && !isInDiffEditor && !virtualWorkspace && shellExecutionSupported"
},
{
"command": "python.execInNewTerminal",
"group": "navigation@0",
"title": "%python.command.python.execInNewTerminal.title%",
"when": "resourceLangId == python && !isInDiffEditor && !virtualWorkspace && shellExecutionSupported"
},
{
"command": "python.debugInTerminal",
"group": "navigation@1",
Expand Down
1 change: 1 addition & 0 deletions package.nls.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
"python.command.python.execInTerminal.title": "Run Python File in Terminal",
"python.command.python.debugInTerminal.title": "Debug Python File",
"python.command.python.execInTerminalIcon.title": "Run Python File",
"python.command.python.execInNewTerminal.title": "Run Python File in Separate Terminal",
"python.command.python.setInterpreter.title": "Select Interpreter",
"python.command.python.clearWorkspaceInterpreter.title": "Clear Workspace Interpreter Setting",
"python.command.python.viewOutput.title": "Show Output",
Expand Down
1 change: 1 addition & 0 deletions src/client/common/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ export namespace Commands {
export const Enable_SourceMap_Support = 'python.enableSourceMapSupport';
export const Exec_In_Terminal = 'python.execInTerminal';
export const Exec_In_Terminal_Icon = 'python.execInTerminal-icon';
export const Exec_In_Separate_Terminal = 'python.execInNewTerminal';
export const Exec_Selection_In_Django_Shell = 'python.execSelectionInDjangoShell';
export const Exec_Selection_In_Terminal = 'python.execSelectionInTerminal';
export const GetSelectedInterpreterPath = 'python.interpreterPath';
Expand Down
16 changes: 11 additions & 5 deletions src/client/common/terminal/factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,14 @@ export class TerminalServiceFactory implements ITerminalServiceFactory {
) {
this.terminalServices = new Map<string, TerminalService>();
}
public getTerminalService(options: TerminalCreationOptions): ITerminalService {
public getTerminalService(options: TerminalCreationOptions & { newTerminalPerFile?: boolean }): ITerminalService {
const resource = options?.resource;
const title = options?.title;
let terminalTitle = typeof title === 'string' && title.trim().length > 0 ? title.trim() : 'Python';
const interpreter = options?.interpreter;
const id = this.getTerminalId(terminalTitle, resource, interpreter);
const id = this.getTerminalId(terminalTitle, resource, interpreter, options.newTerminalPerFile);
if (!this.terminalServices.has(id)) {
if (this.terminalServices.size >= 1 && resource) {
if (resource && options.newTerminalPerFile) {
terminalTitle = `${terminalTitle}: ${path.basename(resource.fsPath).replace('.py', '')}`;
}
options.title = terminalTitle;
Expand All @@ -51,13 +51,19 @@ export class TerminalServiceFactory implements ITerminalServiceFactory {
title = typeof title === 'string' && title.trim().length > 0 ? title.trim() : 'Python';
return new TerminalService(this.serviceContainer, { resource, title });
}
private getTerminalId(title: string, resource?: Uri, interpreter?: PythonEnvironment): string {
private getTerminalId(
title: string,
resource?: Uri,
interpreter?: PythonEnvironment,
newTerminalPerFile?: boolean,
): string {
if (!resource && !interpreter) {
return title;
}
const workspaceFolder = this.serviceContainer
.get<IWorkspaceService>(IWorkspaceService)
.getWorkspaceFolder(resource || undefined);
return `${title}:${workspaceFolder?.uri.fsPath || ''}:${interpreter?.path}:${resource?.fsPath || ''}`;
const fileId = resource && newTerminalPerFile ? resource.fsPath : '';
return `${title}:${workspaceFolder?.uri.fsPath || ''}:${interpreter?.path}:${fileId}`;
}
}
2 changes: 1 addition & 1 deletion src/client/common/terminal/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ export interface ITerminalServiceFactory {
* @returns {ITerminalService}
* @memberof ITerminalServiceFactory
*/
getTerminalService(options: TerminalCreationOptions): ITerminalService;
getTerminalService(options: TerminalCreationOptions & { newTerminalPerFile?: boolean }): ITerminalService;
createTerminalService(resource?: Uri, title?: string): ITerminalService;
}

Expand Down
6 changes: 6 additions & 0 deletions src/client/telemetry/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -825,6 +825,12 @@ export interface IEventNamePropertyMapping {
* @type {('command' | 'icon')}
*/
trigger?: 'command' | 'icon';
/**
* Whether user chose to execute this Python file in a separate terminal or not.
*
* @type {boolean}
*/
newTerminalPerFile?: boolean;
};
/**
* Telemetry Event sent when user executes code against Django Shell.
Expand Down
56 changes: 35 additions & 21 deletions src/client/terminals/codeExecution/codeExecutionManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,25 +36,31 @@ export class CodeExecutionManager implements ICodeExecutionManager {
}

public registerCommands() {
[Commands.Exec_In_Terminal, Commands.Exec_In_Terminal_Icon].forEach((cmd) => {
this.disposableRegistry.push(
this.commandManager.registerCommand(cmd as any, async (file: Resource) => {
const interpreterService = this.serviceContainer.get<IInterpreterService>(IInterpreterService);
const interpreter = await interpreterService.getActiveInterpreter(file);
if (!interpreter) {
this.commandManager.executeCommand(Commands.TriggerEnvironmentSelection, file).then(noop, noop);
return;
}
const trigger = cmd === Commands.Exec_In_Terminal ? 'command' : 'icon';
await this.executeFileInTerminal(file, trigger)
.then(() => {
if (this.shouldTerminalFocusOnStart(file))
this.commandManager.executeCommand('workbench.action.terminal.focus');
[Commands.Exec_In_Terminal, Commands.Exec_In_Terminal_Icon, Commands.Exec_In_Separate_Terminal].forEach(
(cmd) => {
this.disposableRegistry.push(
this.commandManager.registerCommand(cmd as any, async (file: Resource) => {
const interpreterService = this.serviceContainer.get<IInterpreterService>(IInterpreterService);
const interpreter = await interpreterService.getActiveInterpreter(file);
if (!interpreter) {
this.commandManager
.executeCommand(Commands.TriggerEnvironmentSelection, file)
.then(noop, noop);
return;
}
const trigger = cmd === Commands.Exec_In_Terminal ? 'command' : 'icon';
await this.executeFileInTerminal(file, trigger, {
newTerminalPerFile: cmd === Commands.Exec_In_Separate_Terminal,
})
.catch((ex) => traceError('Failed to execute file in terminal', ex));
}),
);
});
.then(() => {
if (this.shouldTerminalFocusOnStart(file))
this.commandManager.executeCommand('workbench.action.terminal.focus');
})
.catch((ex) => traceError('Failed to execute file in terminal', ex));
}),
);
},
);
this.disposableRegistry.push(
this.commandManager.registerCommand(Commands.Exec_Selection_In_Terminal as any, async (file: Resource) => {
const interpreterService = this.serviceContainer.get<IInterpreterService>(IInterpreterService);
Expand Down Expand Up @@ -87,8 +93,16 @@ export class CodeExecutionManager implements ICodeExecutionManager {
),
);
}
private async executeFileInTerminal(file: Resource, trigger: 'command' | 'icon') {
sendTelemetryEvent(EventName.EXECUTION_CODE, undefined, { scope: 'file', trigger });
private async executeFileInTerminal(
file: Resource,
trigger: 'command' | 'icon',
options?: { newTerminalPerFile: boolean },
): Promise<void> {
sendTelemetryEvent(EventName.EXECUTION_CODE, undefined, {
scope: 'file',
trigger,
newTerminalPerFile: options?.newTerminalPerFile,
});
const codeExecutionHelper = this.serviceContainer.get<ICodeExecutionHelper>(ICodeExecutionHelper);
file = file instanceof Uri ? file : undefined;
let fileToExecute = file ? file : await codeExecutionHelper.getFileToExecute();
Expand All @@ -110,7 +124,7 @@ export class CodeExecutionManager implements ICodeExecutionManager {
}

const executionService = this.serviceContainer.get<ICodeExecutionService>(ICodeExecutionService, 'standard');
await executionService.executeFile(fileToExecute);
await executionService.executeFile(fileToExecute, options);
}

@captureTelemetry(EventName.EXECUTION_CODE, { scope: 'selection' }, false)
Expand Down
19 changes: 9 additions & 10 deletions src/client/terminals/codeExecution/terminalCodeExecution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import { ICodeExecutionService } from '../../terminals/types';
export class TerminalCodeExecutionProvider implements ICodeExecutionService {
private hasRanOutsideCurrentDrive = false;
protected terminalTitle!: string;
private replActive = new Map<string, Promise<boolean>>();
private replActive?: Promise<boolean>;
constructor(
@inject(ITerminalServiceFactory) protected readonly terminalServiceFactory: ITerminalServiceFactory,
@inject(IConfigurationService) protected readonly configurationService: IConfigurationService,
Expand All @@ -29,13 +29,13 @@ export class TerminalCodeExecutionProvider implements ICodeExecutionService {
@inject(IInterpreterService) protected readonly interpreterService: IInterpreterService,
) {}

public async executeFile(file: Uri) {
public async executeFile(file: Uri, options?: { newTerminalPerFile: boolean }) {
await this.setCwdForFileExecution(file);
const { command, args } = await this.getExecuteFileArgs(file, [
file.fsPath.fileToCommandArgumentForPythonExt(),
]);

await this.getTerminalService(file).sendCommand(command, args);
await this.getTerminalService(file, options).sendCommand(command, args);
}

public async execute(code: string, resource?: Uri): Promise<void> {
Expand All @@ -48,26 +48,24 @@ export class TerminalCodeExecutionProvider implements ICodeExecutionService {
}
public async initializeRepl(resource?: Uri) {
const terminalService = this.getTerminalService(resource);
let replActive = this.replActive.get(resource?.fsPath || '');
if (replActive && (await replActive)) {
if (this.replActive && (await this.replActive)) {
await terminalService.show();
return;
}
replActive = new Promise<boolean>(async (resolve) => {
this.replActive = new Promise<boolean>(async (resolve) => {
const replCommandArgs = await this.getExecutableInfo(resource);
terminalService.sendCommand(replCommandArgs.command, replCommandArgs.args);

// Give python repl time to start before we start sending text.
setTimeout(() => resolve(true), 1000);
});
this.replActive.set(resource?.fsPath || '', replActive);
this.disposables.push(
terminalService.onDidCloseTerminal(() => {
this.replActive.delete(resource?.fsPath || '');
this.replActive = undefined;
}),
);

await replActive;
await this.replActive;
}

public async getExecutableInfo(resource?: Uri, args: string[] = []): Promise<PythonExecInfo> {
Expand All @@ -83,10 +81,11 @@ export class TerminalCodeExecutionProvider implements ICodeExecutionService {
public async getExecuteFileArgs(resource?: Uri, executeArgs: string[] = []): Promise<PythonExecInfo> {
return this.getExecutableInfo(resource, executeArgs);
}
private getTerminalService(resource?: Uri): ITerminalService {
private getTerminalService(resource?: Uri, options?: { newTerminalPerFile: boolean }): ITerminalService {
return this.terminalServiceFactory.getTerminalService({
resource,
title: this.terminalTitle,
newTerminalPerFile: options?.newTerminalPerFile,
});
}
private async setCwdForFileExecution(file: Uri) {
Expand Down
2 changes: 1 addition & 1 deletion src/client/terminals/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ export const ICodeExecutionService = Symbol('ICodeExecutionService');

export interface ICodeExecutionService {
execute(code: string, resource?: Uri): Promise<void>;
executeFile(file: Uri): Promise<void>;
executeFile(file: Uri, options?: { newTerminalPerFile: boolean }): Promise<void>;
initializeRepl(resource?: Uri): Promise<void>;
}

Expand Down
47 changes: 46 additions & 1 deletion src/test/common/terminals/factory.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ suite('Terminal Service Factory', () => {
expect(notSameAsThirdInstance).to.not.equal(true, 'Instances are the same');
});

test('Ensure different terminal is returned when using different resources from the same workspace', () => {

Choose a reason for hiding this comment

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

Details

test('Ensure same terminal is returned when using different resources from the same workspace', () => {
const file1A = Uri.file('1a');
const file2A = Uri.file('2a');
const fileB = Uri.file('b');
Expand All @@ -130,6 +130,51 @@ suite('Terminal Service Factory', () => {
const terminalForFile2A = factory.getTerminalService({ resource: file2A }) as SynchronousTerminalService;
const terminalForFileB = factory.getTerminalService({ resource: fileB }) as SynchronousTerminalService;

const terminalsAreSameForWorkspaceA = terminalForFile1A.terminalService === terminalForFile2A.terminalService;
expect(terminalsAreSameForWorkspaceA).to.equal(true, 'Instances are not the same for Workspace A');

const terminalsForWorkspaceABAreDifferent =
terminalForFile1A.terminalService === terminalForFileB.terminalService;
expect(terminalsForWorkspaceABAreDifferent).to.equal(
false,
'Instances should be different for different workspaces',
);
});

test('When `newTerminalPerFile` is true, ensure different terminal is returned when using different resources from the same workspace', () => {
const file1A = Uri.file('1a');
const file2A = Uri.file('2a');
const fileB = Uri.file('b');
const workspaceUriA = Uri.file('A');
const workspaceUriB = Uri.file('B');
const workspaceFolderA = TypeMoq.Mock.ofType<WorkspaceFolder>();
workspaceFolderA.setup((w) => w.uri).returns(() => workspaceUriA);
const workspaceFolderB = TypeMoq.Mock.ofType<WorkspaceFolder>();
workspaceFolderB.setup((w) => w.uri).returns(() => workspaceUriB);

workspaceService
.setup((w) => w.getWorkspaceFolder(TypeMoq.It.isValue(file1A)))
.returns(() => workspaceFolderA.object);
workspaceService
.setup((w) => w.getWorkspaceFolder(TypeMoq.It.isValue(file2A)))
.returns(() => workspaceFolderA.object);
workspaceService
.setup((w) => w.getWorkspaceFolder(TypeMoq.It.isValue(fileB)))
.returns(() => workspaceFolderB.object);

const terminalForFile1A = factory.getTerminalService({
resource: file1A,
newTerminalPerFile: true,
}) as SynchronousTerminalService;
const terminalForFile2A = factory.getTerminalService({
resource: file2A,
newTerminalPerFile: true,
}) as SynchronousTerminalService;
const terminalForFileB = factory.getTerminalService({
resource: fileB,
newTerminalPerFile: true,
}) as SynchronousTerminalService;

const terminalsAreSameForWorkspaceA = terminalForFile1A.terminalService === terminalForFile2A.terminalService;
expect(terminalsAreSameForWorkspaceA).to.equal(false, 'Instances are the same for Workspace A');

Expand Down
25 changes: 17 additions & 8 deletions src/test/terminals/codeExecution/codeExecutionManager.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,12 +77,15 @@ suite('Terminal - Code Execution Manager', () => {
executionManager.registerCommands();

const sorted = registered.sort();
expect(sorted).to.deep.equal([
Commands.Exec_In_Terminal,
Commands.Exec_In_Terminal_Icon,
Commands.Exec_Selection_In_Django_Shell,
Commands.Exec_Selection_In_Terminal,
]);
expect(sorted).to.deep.equal(
[
Commands.Exec_In_Separate_Terminal,
Commands.Exec_In_Terminal,
Commands.Exec_In_Terminal_Icon,
Commands.Exec_Selection_In_Django_Shell,
Commands.Exec_Selection_In_Terminal,
].sort(),
);
});

test('Ensure executeFileInterTerminal will do nothing if no file is avialble', async () => {
Expand Down Expand Up @@ -135,7 +138,10 @@ suite('Terminal - Code Execution Manager', () => {
const fileToExecute = Uri.file('x');
await commandHandler!(fileToExecute);
helper.verify(async (h) => h.getFileToExecute(), TypeMoq.Times.never());
executionService.verify(async (e) => e.executeFile(TypeMoq.It.isValue(fileToExecute)), TypeMoq.Times.once());
executionService.verify(
async (e) => e.executeFile(TypeMoq.It.isValue(fileToExecute), TypeMoq.It.isAny()),
TypeMoq.Times.once(),
);
});

test('Ensure executeFileInterTerminal will use active file', async () => {
Expand Down Expand Up @@ -164,7 +170,10 @@ suite('Terminal - Code Execution Manager', () => {
.returns(() => executionService.object);

await commandHandler!(fileToExecute);
executionService.verify(async (e) => e.executeFile(TypeMoq.It.isValue(fileToExecute)), TypeMoq.Times.once());
executionService.verify(
async (e) => e.executeFile(TypeMoq.It.isValue(fileToExecute), TypeMoq.It.isAny()),
TypeMoq.Times.once(),
);
});

async function testExecutionOfSelectionWithoutAnyActiveDocument(commandId: string, executionSericeId: string) {
Expand Down