Make shift+enter work in DS and non-DS scenarios (#9445)

rchiodo · rchiodo · commit 6346a6f636df · 2020-01-08T11:24:33.000-08:00
* Conda run doesn't work for normalizing lines or starting a repl

* Add news entries

* Fix the rest of the linter errors

* Fix unit tests

* Fix helper tests
diff --git a/news/2 Fixes/9437.md b/news/2 Fixes/9437.md
@@ -0,0 +1 @@
+Shift+Enter can no longer send multiple lines to the interactive window.
diff --git a/news/2 Fixes/9439.md b/news/2 Fixes/9439.md
@@ -0,0 +1 @@
+Shift+Enter can no longer run code in the terminal.
diff --git a/src/client/terminals/codeExecution/djangoShellCodeExecution.ts b/src/client/terminals/codeExecution/djangoShellCodeExecution.ts
@@ -9,7 +9,7 @@ import { Disposable, Uri } from 'vscode';
 import { ICommandManager, IDocumentManager, IWorkspaceService } from '../../common/application/types';
 import '../../common/extensions';
 import { IFileSystem, IPlatformService } from '../../common/platform/types';
-import { IPythonExecutionFactory, PythonExecutionInfo } from '../../common/process/types';
+import { PythonExecutionInfo } from '../../common/process/types';
 import { ITerminalServiceFactory } from '../../common/terminal/types';
 import { IConfigurationService, IDisposableRegistry } from '../../common/types';
 import { DjangoContextInitializer } from './djangoContext';
@@ -25,10 +25,9 @@ export class DjangoShellCodeExecutionProvider extends TerminalCodeExecutionProvi
         @inject(IPlatformService) platformService: IPlatformService,
         @inject(ICommandManager) commandManager: ICommandManager,
         @inject(IFileSystem) fileSystem: IFileSystem,
-        @inject(IPythonExecutionFactory) pythonExecFactory: IPythonExecutionFactory,
         @inject(IDisposableRegistry) disposableRegistry: Disposable[]
     ) {
-        super(terminalServiceFactory, configurationService, workspace, disposableRegistry, platformService, pythonExecFactory);
+        super(terminalServiceFactory, configurationService, workspace, disposableRegistry, platformService);
         this.terminalTitle = 'Django Shell';
         disposableRegistry.push(new DjangoContextInitializer(documentManager, workspace, fileSystem, commandManager));
     }
diff --git a/src/client/terminals/codeExecution/helper.ts b/src/client/terminals/codeExecution/helper.ts
@@ -1,26 +1,30 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License.
+import '../../common/extensions';
 
 import { inject, injectable } from 'inversify';
 import * as path from 'path';
 import { Range, TextEditor, Uri } from 'vscode';
+
 import { IApplicationShell, IDocumentManager } from '../../common/application/types';
 import { EXTENSION_ROOT_DIR, PYTHON_LANGUAGE } from '../../common/constants';
-import '../../common/extensions';
 import { traceError } from '../../common/logger';
-import { IPythonExecutionFactory } from '../../common/process/types';
+import { IProcessServiceFactory } from '../../common/process/types';
+import { IInterpreterService } from '../../interpreter/contracts';
 import { IServiceContainer } from '../../ioc/types';
 import { ICodeExecutionHelper } from '../types';
 
 @injectable()
 export class CodeExecutionHelper implements ICodeExecutionHelper {
     private readonly documentManager: IDocumentManager;
     private readonly applicationShell: IApplicationShell;
-    private readonly pythonServiceFactory: IPythonExecutionFactory;
+    private readonly processServiceFactory: IProcessServiceFactory;
+    private readonly interpreterService: IInterpreterService;
     constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) {
         this.documentManager = serviceContainer.get<IDocumentManager>(IDocumentManager);
         this.applicationShell = serviceContainer.get<IApplicationShell>(IApplicationShell);
-        this.pythonServiceFactory = serviceContainer.get<IPythonExecutionFactory>(IPythonExecutionFactory);
+        this.processServiceFactory = serviceContainer.get<IProcessServiceFactory>(IProcessServiceFactory);
+        this.interpreterService = serviceContainer.get<IInterpreterService>(IInterpreterService);
     }
     public async normalizeLines(code: string, resource?: Uri): Promise<string> {
         try {
@@ -30,9 +34,10 @@ export class CodeExecutionHelper implements ICodeExecutionHelper {
             // On windows cr is not handled well by python when passing in/out via stdin/stdout.
             // So just remove cr from the input.
             code = code.replace(new RegExp('\\r', 'g'), '');
+            const interpreter = await this.interpreterService.getActiveInterpreter(resource);
+            const processService = await this.processServiceFactory.create(resource);
             const args = [path.join(EXTENSION_ROOT_DIR, 'pythonFiles', 'normalizeForInterpreter.py'), code];
-            const processService = await this.pythonServiceFactory.create({ resource });
-            const proc = await processService.exec(args, { throwOnStdErr: true });
+            const proc = await processService.exec(interpreter?.path || 'python', args, { throwOnStdErr: true });
 
             return proc.stdout;
         } catch (ex) {
diff --git a/src/client/terminals/codeExecution/repl.ts b/src/client/terminals/codeExecution/repl.ts
@@ -7,7 +7,6 @@ import { inject, injectable } from 'inversify';
 import { Disposable } from 'vscode';
 import { IWorkspaceService } from '../../common/application/types';
 import { IPlatformService } from '../../common/platform/types';
-import { IPythonExecutionFactory } from '../../common/process/types';
 import { ITerminalServiceFactory } from '../../common/terminal/types';
 import { IConfigurationService, IDisposableRegistry } from '../../common/types';
 import { TerminalCodeExecutionProvider } from './terminalCodeExecution';
@@ -18,11 +17,10 @@ export class ReplProvider extends TerminalCodeExecutionProvider {
         @inject(ITerminalServiceFactory) terminalServiceFactory: ITerminalServiceFactory,
         @inject(IConfigurationService) configurationService: IConfigurationService,
         @inject(IWorkspaceService) workspace: IWorkspaceService,
-        @inject(IPythonExecutionFactory) pythonExecFactory: IPythonExecutionFactory,
         @inject(IDisposableRegistry) disposableRegistry: Disposable[],
         @inject(IPlatformService) platformService: IPlatformService
     ) {
-        super(terminalServiceFactory, configurationService, workspace, disposableRegistry, platformService, pythonExecFactory);
+        super(terminalServiceFactory, configurationService, workspace, disposableRegistry, platformService);
         this.terminalTitle = 'REPL';
     }
 }
diff --git a/src/client/terminals/codeExecution/terminalCodeExecution.ts b/src/client/terminals/codeExecution/terminalCodeExecution.ts
@@ -9,7 +9,7 @@ import { Disposable, Uri } from 'vscode';
 import { IWorkspaceService } from '../../common/application/types';
 import '../../common/extensions';
 import { IPlatformService } from '../../common/platform/types';
-import { IPythonExecutionFactory, PythonExecutionInfo } from '../../common/process/types';
+import { PythonExecutionInfo } from '../../common/process/types';
 import { ITerminalService, ITerminalServiceFactory } from '../../common/terminal/types';
 import { IConfigurationService, IDisposableRegistry } from '../../common/types';
 import { ICodeExecutionService } from '../../terminals/types';
@@ -24,8 +24,7 @@ export class TerminalCodeExecutionProvider implements ICodeExecutionService {
         @inject(IConfigurationService) protected readonly configurationService: IConfigurationService,
         @inject(IWorkspaceService) protected readonly workspace: IWorkspaceService,
         @inject(IDisposableRegistry) protected readonly disposables: Disposable[],
-        @inject(IPlatformService) protected readonly platformService: IPlatformService,
-        @inject(IPythonExecutionFactory) private readonly pythonExecFactory: IPythonExecutionFactory
+        @inject(IPlatformService) protected readonly platformService: IPlatformService
     ) {}
 
     public async executeFile(file: Uri) {
@@ -64,11 +63,6 @@ export class TerminalCodeExecutionProvider implements ICodeExecutionService {
         const command = pythonSettings.pythonPath;
         const launchArgs = pythonSettings.terminal.launchArgs;
 
-        const condaExecutionService = await this.pythonExecFactory.createCondaExecutionService(command, undefined, resource);
-        if (condaExecutionService) {
-            return condaExecutionService.getExecutionInfo([...launchArgs, ...args]);
-        }
-
         const isWindows = this.platformService.isWindows;
 
         return {
diff --git a/src/test/terminals/codeExecution/djangoShellCodeExect.unit.test.ts b/src/test/terminals/codeExecution/djangoShellCodeExect.unit.test.ts
@@ -37,10 +37,10 @@ suite('Terminal - Django Shell Code Execution', () => {
         workspace
             .setup(c => c.onDidChangeWorkspaceFolders(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny()))
             .returns(() => {
-            return {
-                dispose: () => void 0
-            };
-        });
+                return {
+                    dispose: () => void 0
+                };
+            });
         platform = TypeMoq.Mock.ofType<IPlatformService>();
         const documentManager = TypeMoq.Mock.ofType<IDocumentManager>();
         const commandManager = TypeMoq.Mock.ofType<ICommandManager>();
@@ -54,7 +54,6 @@ suite('Terminal - Django Shell Code Execution', () => {
             platform.object,
             commandManager.object,
             fileSystem.object,
-            pythonExecutionFactory.object,
             disposables
         );
 
@@ -74,7 +73,14 @@ suite('Terminal - Django Shell Code Execution', () => {
         disposables = [];
     });
 
-    async function testReplCommandArguments(isWindows: boolean, pythonPath: string, expectedPythonPath: string, terminalArgs: string[], expectedTerminalArgs: string[], resource?: Uri) {
+    async function testReplCommandArguments(
+        isWindows: boolean,
+        pythonPath: string,
+        expectedPythonPath: string,
+        terminalArgs: string[],
+        expectedTerminalArgs: string[],
+        resource?: Uri
+    ) {
         platform.setup(p => p.isWindows).returns(() => isWindows);
         settings.setup(s => s.pythonPath).returns(() => pythonPath);
         terminalSettings.setup(t => t.launchArgs).returns(() => terminalArgs);
@@ -179,16 +185,16 @@ suite('Terminal - Django Shell Code Execution', () => {
         const serviceContainer = TypeMoq.Mock.ofType<IServiceContainer>();
         const processService = TypeMoq.Mock.ofType<IProcessService>();
         const condaExecutionService = new CondaExecutionService(serviceContainer.object, processService.object, pythonPath, condaFile, condaEnv);
-        const hasEnvName = condaEnv.name !== '';
-        const condaArgs = ['run', ...(hasEnvName ? ['-n', condaEnv.name] : ['-p', condaEnv.path]), 'python'];
-        const expectedTerminalArgs = [...condaArgs, ...terminalArgs, 'manage.py', 'shell'];
-        pythonExecutionFactory.setup(p => p.createCondaExecutionService(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve(condaExecutionService));
+        const expectedTerminalArgs = [...terminalArgs, 'manage.py', 'shell'];
+        pythonExecutionFactory
+            .setup(p => p.createCondaExecutionService(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny()))
+            .returns(() => Promise.resolve(condaExecutionService));
 
         const replCommandArgs = await (executor as DjangoShellCodeExecutionProvider).getExecutableInfo(resource);
 
         expect(replCommandArgs).not.to.be.an('undefined', 'Conda command args are undefined');
-        expect(replCommandArgs.command).to.be.equal(condaFile, 'Incorrect conda path');
-        expect(replCommandArgs.args).to.be.deep.equal(expectedTerminalArgs, 'Incorrect conda arguments');
+        expect(replCommandArgs.command).to.be.equal(pythonPath, 'Repl should use python not conda');
+        expect(replCommandArgs.args).to.be.deep.equal(expectedTerminalArgs, 'Incorrect terminal arguments');
     }
 
     test('Ensure conda args including env name are passed when using a conda environment with a name', async () => {
diff --git a/src/test/terminals/codeExecution/helper.test.ts b/src/test/terminals/codeExecution/helper.test.ts
@@ -7,16 +7,18 @@ import { expect } from 'chai';
 import * as fs from 'fs-extra';
 import { EOL } from 'os';
 import * as path from 'path';
+import { SemVer } from 'semver';
 import * as TypeMoq from 'typemoq';
 import { Range, Selection, TextDocument, TextEditor, TextLine, Uri } from 'vscode';
 import { IApplicationShell, IDocumentManager } from '../../../client/common/application/types';
 import { EXTENSION_ROOT_DIR, PYTHON_LANGUAGE } from '../../../client/common/constants';
 import '../../../client/common/extensions';
 import { BufferDecoder } from '../../../client/common/process/decoder';
 import { ProcessService } from '../../../client/common/process/proc';
-import { IPythonExecutionFactory, IPythonExecutionService } from '../../../client/common/process/types';
-import { OSType } from '../../../client/common/utils/platform';
+import { IProcessService, IProcessServiceFactory } from '../../../client/common/process/types';
+import { Architecture, OSType } from '../../../client/common/utils/platform';
 import { IEnvironmentVariablesProvider } from '../../../client/common/variables/types';
+import { IInterpreterService, InterpreterType, PythonInterpreter } from '../../../client/interpreter/contracts';
 import { IServiceContainer } from '../../../client/ioc/types';
 import { CodeExecutionHelper } from '../../../client/terminals/codeExecution/helper';
 import { ICodeExecutionHelper } from '../../../client/terminals/types';
@@ -31,19 +33,33 @@ suite('Terminal - Code Execution Helper', () => {
     let helper: ICodeExecutionHelper;
     let document: TypeMoq.IMock<TextDocument>;
     let editor: TypeMoq.IMock<TextEditor>;
-    let pythonService: TypeMoq.IMock<IPythonExecutionService>;
+    let processService: TypeMoq.IMock<IProcessService>;
+    let interpreterService: TypeMoq.IMock<IInterpreterService>;
+    const workingPython: PythonInterpreter = {
+        path: PYTHON_PATH,
+        version: new SemVer('3.6.6-final'),
+        sysVersion: '1.0.0.0',
+        sysPrefix: 'Python',
+        displayName: 'Python',
+        type: InterpreterType.Unknown,
+        architecture: Architecture.x64
+    };
+
     setup(() => {
         const serviceContainer = TypeMoq.Mock.ofType<IServiceContainer>();
         documentManager = TypeMoq.Mock.ofType<IDocumentManager>();
         applicationShell = TypeMoq.Mock.ofType<IApplicationShell>();
         const envVariablesProvider = TypeMoq.Mock.ofType<IEnvironmentVariablesProvider>();
-        pythonService = TypeMoq.Mock.ofType<IPythonExecutionService>();
+        processService = TypeMoq.Mock.ofType<IProcessService>();
+        interpreterService = TypeMoq.Mock.ofType<IInterpreterService>();
         // tslint:disable-next-line:no-any
-        pythonService.setup((x: any) => x.then).returns(() => undefined);
+        processService.setup((x: any) => x.then).returns(() => undefined);
+        interpreterService.setup(i => i.getActiveInterpreter(TypeMoq.It.isAny())).returns(() => Promise.resolve(workingPython));
+        const processServiceFactory = TypeMoq.Mock.ofType<IProcessServiceFactory>();
+        processServiceFactory.setup(p => p.create(TypeMoq.It.isAny())).returns(() => Promise.resolve(processService.object));
         envVariablesProvider.setup(e => e.getEnvironmentVariables(TypeMoq.It.isAny())).returns(() => Promise.resolve({}));
-        const pythonExecFactory = TypeMoq.Mock.ofType<IPythonExecutionFactory>();
-        pythonExecFactory.setup(p => p.create(TypeMoq.It.isAny())).returns(() => Promise.resolve(pythonService.object));
-        serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IPythonExecutionFactory), TypeMoq.It.isAny())).returns(() => pythonExecFactory.object);
+        serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IProcessServiceFactory), TypeMoq.It.isAny())).returns(() => processServiceFactory.object);
+        serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IInterpreterService), TypeMoq.It.isAny())).returns(() => interpreterService.object);
         serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IDocumentManager), TypeMoq.It.isAny())).returns(() => documentManager.object);
         serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IApplicationShell), TypeMoq.It.isAny())).returns(() => applicationShell.object);
         serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IEnvironmentVariablesProvider), TypeMoq.It.isAny())).returns(() => envVariablesProvider.object);
@@ -56,10 +72,10 @@ suite('Terminal - Code Execution Helper', () => {
 
     async function ensureBlankLinesAreRemoved(source: string, expectedSource: string) {
         const actualProcessService = new ProcessService(new BufferDecoder());
-        pythonService
-            .setup(p => p.exec(TypeMoq.It.isAny(), TypeMoq.It.isAny()))
-            .returns((args, options) => {
-                return actualProcessService.exec.apply(actualProcessService, [PYTHON_PATH, args, options]);
+        processService
+            .setup(p => p.exec(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny()))
+            .returns((file, args, options) => {
+                return actualProcessService.exec.apply(actualProcessService, [file, args, options]);
             });
         const normalizedZCode = await helper.normalizeLines(source);
         // In case file has been saved with different line endings.
@@ -81,9 +97,9 @@ suite('Terminal - Code Execution Helper', () => {
     test('Ensure there are no multiple-CR elements in the normalized code.', async () => {
         const code = ['import sys', '', '', '', 'print(sys.executable)', '', 'print("1234")', '', '', 'print(1)', 'print(2)'];
         const actualProcessService = new ProcessService(new BufferDecoder());
-        pythonService
-            .setup(p => p.exec(TypeMoq.It.isAny(), TypeMoq.It.isAny()))
-            .returns((args, options) => {
+        processService
+            .setup(p => p.exec(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny()))
+            .returns((_file, args, options) => {
                 return actualProcessService.exec.apply(actualProcessService, [PYTHON_PATH, args, options]);
             });
         const normalizedCode = await helper.normalizeLines(code.join(EOL));
diff --git a/src/test/terminals/codeExecution/terminalCodeExec.unit.test.ts b/src/test/terminals/codeExecution/terminalCodeExec.unit.test.ts

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+Shift+Enter can no longer send multiple lines to the interactive window.`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+Shift+Enter can no longer run code in the terminal.`