Refactor formatters to use new execution framework (#426)

Fixes #352
Refactor code to use the new execution framework when launching processes

src/client/formatters/autoPep8Formatter.ts

@@ -15,17 +15,15 @@ export class AutoPep8Formatter extends BaseFormatter {
     public formatDocument(document: vscode.TextDocument, options: vscode.FormattingOptions, token: vscode.CancellationToken, range?: vscode.Range): Thenable<vscode.TextEdit[]> {
         const stopWatch = new StopWatch();
         const settings = PythonSettings.getInstance(document.uri);
-        const autopep8Path = settings.formatting.autopep8Path;
-        const autoPep8Args = Array.isArray(settings.formatting.autopep8Args) ? settings.formatting.autopep8Args : [];
-        const hasCustomArgs = autoPep8Args.length > 0;
+        const hasCustomArgs = Array.isArray(settings.formatting.autopep8Args) && settings.formatting.autopep8Args.length > 0;
         const formatSelection = range ? !range.isEmpty : false;
 
-        autoPep8Args.push('--diff');
+        const autoPep8Args = ['--diff'];
         if (formatSelection) {
             // tslint:disable-next-line:no-non-null-assertion
             autoPep8Args.push(...['--line-range', (range!.start.line + 1).toString(), (range!.end.line + 1).toString()]);
         }
-        const promise = super.provideDocumentFormattingEdits(document, options, token, autopep8Path, autoPep8Args);
+        const promise = super.provideDocumentFormattingEdits(document, options, token, autoPep8Args);
         sendTelemetryWhenDone(FORMAT, promise, stopWatch, { tool: 'autoppep8', hasCustomArgs, formatSelection });
         return promise;
     }

src/client/formatters/baseFormatter.ts

@@ -1,18 +1,22 @@
 import * as fs from 'fs';
 import * as path from 'path';
 import * as vscode from 'vscode';
-import { OutputChannel, Uri } from 'vscode';
+import { OutputChannel, TextEdit, Uri } from 'vscode';
 import { STANDARD_OUTPUT_CHANNEL } from '../common/constants';
 import { isNotInstalledError } from '../common/helpers';
+import { IProcessService, IPythonExecutionFactory } from '../common/process/types';
 import { IInstaller, IOutputChannel, Product } from '../common/types';
+import { IEnvironmentVariablesProvider } from '../common/variables/types';
 import { IServiceContainer } from '../ioc/types';
 import { getTempFileWithDocumentContents, getTextEditsFromPatch } from './../common/editor';
-import { execPythonFile } from './../common/utils';
+import { IFormatterHelper } from './types';
 
 export abstract class BaseFormatter {
     protected readonly outputChannel: OutputChannel;
+    private readonly helper: IFormatterHelper;
     constructor(public Id: string, private product: Product, private serviceContainer: IServiceContainer) {
         this.outputChannel = this.serviceContainer.get<OutputChannel>(IOutputChannel, STANDARD_OUTPUT_CHANNEL);
+        this.helper = this.serviceContainer.get<IFormatterHelper>(IFormatterHelper);
     }
 
     public abstract formatDocument(document: vscode.TextDocument, options: vscode.FormattingOptions, token: vscode.CancellationToken, range?: vscode.Range): Thenable<vscode.TextEdit[]>;
@@ -32,58 +36,72 @@ export abstract class BaseFormatter {
         }
         return vscode.Uri.file(__dirname);
     }
-    protected provideDocumentFormattingEdits(document: vscode.TextDocument, options: vscode.FormattingOptions, token: vscode.CancellationToken, command: string, args: string[], cwd?: string): Thenable<vscode.TextEdit[]> {
+    protected async provideDocumentFormattingEdits(document: vscode.TextDocument, options: vscode.FormattingOptions, token: vscode.CancellationToken, args: string[], cwd?: string): Promise<vscode.TextEdit[]> {
         this.outputChannel.clear();
         if (typeof cwd !== 'string' || cwd.length === 0) {
             cwd = this.getWorkspaceUri(document).fsPath;
         }
 
-        // autopep8 and yapf have the ability to read from the process input stream and return the formatted code out of the output stream
-        // However they don't support returning the diff of the formatted text when reading data from the input stream
+        // autopep8 and yapf have the ability to read from the process input stream and return the formatted code out of the output stream.
+        // However they don't support returning the diff of the formatted text when reading data from the input stream.
         // Yes getting text formatted that way avoids having to create a temporary file, however the diffing will have
-        // to be done here in node (extension), i.e. extension cpu, i.e. les responsive solution
+        // to be done here in node (extension), i.e. extension cpu, i.e. les responsive solution.
         const tmpFileCreated = document.isDirty;
         const filePromise = tmpFileCreated ? getTempFileWithDocumentContents(document) : Promise.resolve(document.fileName);
-        const promise = filePromise.then(filePath => {
-            if (token && token.isCancellationRequested) {
-                return [filePath, ''];
-            }
-            return Promise.all<string>([Promise.resolve(filePath), execPythonFile(document.uri, command, args.concat([filePath]), cwd!)]);
-        }).then(data => {
-            // Delete the temporary file created
-            if (tmpFileCreated) {
-                fs.unlink(data[0]);
-            }
-            if (token && token.isCancellationRequested) {
-                return [];
-            }
-            return getTextEditsFromPatch(document.getText(), data[1]);
-        }).catch(error => {
-            this.handleError(this.Id, command, error, document.uri);
+        const filePath = await filePromise;
+        if (token && token.isCancellationRequested) {
             return [];
-        });
+        }
+
+        let executionPromise: Promise<string>;
+        const executionInfo = this.helper.getExecutionInfo(this.product, args, document.uri);
+        // Check if required to run as a module or executable.
+        if (executionInfo.moduleName) {
+            executionPromise = this.serviceContainer.get<IPythonExecutionFactory>(IPythonExecutionFactory).create(document.uri)
+                .then(pythonExecutionService => pythonExecutionService.execModule(executionInfo.moduleName!, executionInfo.args.concat([filePath]), { cwd, throwOnStdErr: true, token }))
+                .then(output => output.stdout);
+        } else {
+            const executionService = this.serviceContainer.get<IProcessService>(IProcessService);
+            executionPromise = this.serviceContainer.get<IEnvironmentVariablesProvider>(IEnvironmentVariablesProvider).getEnvironmentVariables(true, document.uri)
+                .then(env => executionService.exec(executionInfo.execPath!, executionInfo.args.concat([filePath]), { cwd, env, throwOnStdErr: true, token }))
+                .then(output => output.stdout);
+        }
+
+        const promise = executionPromise
+            .then(data => {
+                if (token && token.isCancellationRequested) {
+                    return [] as TextEdit[];
+                }
+                return getTextEditsFromPatch(document.getText(), data);
+            })
+            .catch(error => {
+                if (token && token.isCancellationRequested) {
+                    return [] as TextEdit[];
+                }
+                // tslint:disable-next-line:no-empty
+                this.handleError(this.Id, error, document.uri).catch(() => { });
+                return [] as TextEdit[];
+            })
+            .then(edits => {
+                // Delete the temporary file created
+                if (tmpFileCreated) {
+                    fs.unlink(filePath);
+                }
+                return edits;
+            });
         vscode.window.setStatusBarMessage(`Formatting with ${this.Id}`, promise);
         return promise;
     }
 
-    protected handleError(expectedFileName: string, fileName: string, error: Error, resource?: Uri) {
+    protected async handleError(expectedFileName: string, error: Error, resource?: Uri) {
         let customError = `Formatting with ${this.Id} failed.`;
 
         if (isNotInstalledError(error)) {
-            // Check if we have some custom arguments such as "pylint --load-plugins pylint_django"
-            // Such settings are no longer supported
-            const stuffAfterFileName = fileName.substring(fileName.toUpperCase().lastIndexOf(expectedFileName) + expectedFileName.length);
-
-            // Ok if we have a space after the file name, this means we have some arguments defined and this isn't supported
-            if (stuffAfterFileName.trim().indexOf(' ') > 0) {
-                // tslint:disable-next-line:prefer-template
-                customError = `Formatting failed, custom arguments in the 'python.formatting.${this.Id}Path' is not supported.\n` +
-                    `Custom arguments to the formatter can be defined in 'python.formatter.${this.Id}Args' setting of settings.json.`;
-            } else {
-                const installer = this.serviceContainer.get<IInstaller>(IInstaller);
+            const installer = this.serviceContainer.get<IInstaller>(IInstaller);
+            const isInstalled = await installer.isInstalled(this.product, resource);
+            if (isInstalled) {
                 customError += `\nYou could either install the '${this.Id}' formatter, turn it off or use another formatter.`;
-                installer.promptToInstall(this.product, resource)
-                    .catch(ex => console.error('Python Extension: promptToInstall', ex));
+                installer.promptToInstall(this.product, resource).catch(ex => console.error('Python Extension: promptToInstall', ex));
             }
         }
 

src/client/formatters/helper.ts

@@ -2,9 +2,11 @@
 // Licensed under the MIT License.
 
 import { injectable } from 'inversify';
+import * as path from 'path';
 import 'reflect-metadata';
-import { IFormattingSettings } from '../common/configSettings';
-import { Product } from '../common/types';
+import { Uri } from 'vscode';
+import { IFormattingSettings, PythonSettings } from '../common/configSettings';
+import { ExecutionInfo, Product } from '../common/types';
 import { FormatterId, FormatterSettingsPropertyNames, IFormatterHelper } from './types';
 
 @injectable()
@@ -25,4 +27,22 @@ export class FormatterHelper implements IFormatterHelper {
             pathName: `${id}Path` as keyof IFormattingSettings
         };
     }
+    public getExecutionInfo(formatter: Product, customArgs: string[], resource?: Uri): ExecutionInfo {
+        const settings = PythonSettings.getInstance(resource);
+        const names = this.getSettingsPropertyNames(formatter);
+
+        const execPath = settings.formatting[names.pathName] as string;
+        let args: string[] = Array.isArray(settings.formatting[names.argsName]) ? settings.formatting[names.argsName] as string[] : [];
+        args = args.concat(customArgs);
+
+        let moduleName: string | undefined;
+
+        // If path information is not available, then treat it as a module,
+        // except for prospector as that needs to be run as an executable (it's a Python package).
+        if (path.basename(execPath) === execPath && formatter !== Product.prospector) {
+            moduleName = execPath;
+        }
+
+        return { execPath, moduleName, args };
+    }
 }

src/client/formatters/types.ts

@@ -1,8 +1,9 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License.
 
+import { Uri } from 'vscode';
 import { IFormattingSettings } from '../common/configSettings';
-import { Product } from '../common/types';
+import { ExecutionInfo, Product } from '../common/types';
 
 export const IFormatterHelper = Symbol('IFormatterHelper');
 
@@ -16,4 +17,5 @@ export type FormatterSettingsPropertyNames = {
 export interface IFormatterHelper {
     translateToId(formatter: Product): FormatterId;
     getSettingsPropertyNames(formatter: Product): FormatterSettingsPropertyNames;
+    getExecutionInfo(formatter: Product, customArgs: string[], resource?: Uri): ExecutionInfo;
 }

src/client/formatters/yapfFormatter.ts

@@ -15,20 +15,18 @@ export class YapfFormatter extends BaseFormatter {
     public formatDocument(document: vscode.TextDocument, options: vscode.FormattingOptions, token: vscode.CancellationToken, range?: vscode.Range): Thenable<vscode.TextEdit[]> {
         const stopWatch = new StopWatch();
         const settings = PythonSettings.getInstance(document.uri);
-        const yapfPath = settings.formatting.yapfPath;
-        const yapfArgs = Array.isArray(settings.formatting.yapfArgs) ? settings.formatting.yapfArgs : [];
-        const hasCustomArgs = yapfArgs.length > 0;
+        const hasCustomArgs = Array.isArray(settings.formatting.yapfArgs) && settings.formatting.yapfArgs.length > 0;
         const formatSelection = range ? !range.isEmpty : false;
 
-        yapfArgs.push('--diff');
+        const yapfArgs = ['--diff'];
         if (formatSelection) {
             // tslint:disable-next-line:no-non-null-assertion
             yapfArgs.push(...['--lines', `${range!.start.line + 1}-${range!.end.line + 1}`]);
         }
         // Yapf starts looking for config file starting from the file path.
         const fallbarFolder = this.getWorkspaceUri(document).fsPath;
         const cwd = this.getDocumentPath(document, fallbarFolder);
-        const promise = super.provideDocumentFormattingEdits(document, options, token, yapfPath, yapfArgs, cwd);
+        const promise = super.provideDocumentFormattingEdits(document, options, token, yapfArgs, cwd);
         sendTelemetryWhenDone(FORMAT, promise, stopWatch, { tool: 'yapf', hasCustomArgs, formatSelection });
         return promise;
     }

src/test/format/extension.format.test.ts

@@ -2,9 +2,8 @@ import * as assert from 'assert';
 import * as fs from 'fs-extra';
 import * as path from 'path';
 import * as vscode from 'vscode';
-import { CancellationTokenSource } from 'vscode';
-import { IProcessService } from '../../client/common/process/types';
-import { execPythonFile } from '../../client/common/utils';
+import { CancellationTokenSource, Uri } from 'vscode';
+import { IProcessService, IPythonExecutionFactory } from '../../client/common/process/types';
 import { AutoPep8Formatter } from '../../client/formatters/autoPep8Formatter';
 import { YapfFormatter } from '../../client/formatters/yapfFormatter';
 import { closeActiveWindows, initialize, initializeTest } from '../initialize';
@@ -29,15 +28,17 @@ suite('Formatting', () => {
 
     suiteSetup(async () => {
         await initialize();
+        initializeDI();
         [autoPep8FileToFormat, autoPep8FileToAutoFormat, yapfFileToFormat, yapfFileToAutoFormat].forEach(file => {
             fs.copySync(originalUnformattedFile, file, { overwrite: true });
         });
         fs.ensureDirSync(path.dirname(autoPep8FileToFormat));
-        const yapf = execPythonFile(workspaceRootPath, 'yapf', [originalUnformattedFile], workspaceRootPath, false);
-        const autoPep8 = execPythonFile(workspaceRootPath, 'autopep8', [originalUnformattedFile], workspaceRootPath, false);
-        await Promise.all<string>([yapf, autoPep8]).then(formattedResults => {
-            formattedYapf = formattedResults[0];
-            formattedAutoPep8 = formattedResults[1];
+        const pythonProcess = await ioc.serviceContainer.get<IPythonExecutionFactory>(IPythonExecutionFactory).create(Uri.file(workspaceRootPath));
+        const yapf = pythonProcess.execModule('yapf', [originalUnformattedFile], { cwd: workspaceRootPath });
+        const autoPep8 = pythonProcess.execModule('autopep8', [originalUnformattedFile], { cwd: workspaceRootPath });
+        await Promise.all([yapf, autoPep8]).then(formattedResults => {
+            formattedYapf = formattedResults[0].stdout;
+            formattedAutoPep8 = formattedResults[1].stdout;
         });
     });
     setup(async () => {
@@ -63,6 +64,7 @@ suite('Formatting', () => {
         ioc.registerCommonTypes();
         ioc.registerVariableTypes();
         ioc.registerUnitTestTypes();
+        ioc.registerFormatterTypes();
 
         // Mocks.
         ioc.registerMockProcessTypes();