diff --git a/src/client/formatters/autoPep8Formatter.ts b/src/client/formatters/autoPep8Formatter.ts index 1fe1a6b08218..11f217641e01 100644 --- a/src/client/formatters/autoPep8Formatter.ts +++ b/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 { 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; } diff --git a/src/client/formatters/baseFormatter.ts b/src/client/formatters/baseFormatter.ts index 0f16c0a8cfc4..6e7a9322cbdb 100644 --- a/src/client/formatters/baseFormatter.ts +++ b/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(IOutputChannel, STANDARD_OUTPUT_CHANNEL); + this.helper = this.serviceContainer.get(IFormatterHelper); } public abstract formatDocument(document: vscode.TextDocument, options: vscode.FormattingOptions, token: vscode.CancellationToken, range?: vscode.Range): Thenable; @@ -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 { + protected async provideDocumentFormattingEdits(document: vscode.TextDocument, options: vscode.FormattingOptions, token: vscode.CancellationToken, args: string[], cwd?: string): Promise { 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([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; + 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).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); + executionPromise = this.serviceContainer.get(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); + const installer = this.serviceContainer.get(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)); } } diff --git a/src/client/formatters/helper.ts b/src/client/formatters/helper.ts index 180ed3433932..ba08df994e96 100644 --- a/src/client/formatters/helper.ts +++ b/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 }; + } } diff --git a/src/client/formatters/types.ts b/src/client/formatters/types.ts index 93d9aad58652..c8a076c10f93 100644 --- a/src/client/formatters/types.ts +++ b/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; } diff --git a/src/client/formatters/yapfFormatter.ts b/src/client/formatters/yapfFormatter.ts index 497629f25efc..4682eb64daec 100644 --- a/src/client/formatters/yapfFormatter.ts +++ b/src/client/formatters/yapfFormatter.ts @@ -15,12 +15,10 @@ export class YapfFormatter extends BaseFormatter { public formatDocument(document: vscode.TextDocument, options: vscode.FormattingOptions, token: vscode.CancellationToken, range?: vscode.Range): Thenable { 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}`]); @@ -28,7 +26,7 @@ export class YapfFormatter extends BaseFormatter { // 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; } diff --git a/src/test/format/extension.format.test.ts b/src/test/format/extension.format.test.ts index 962e1111200b..2c00fa8c6dd6 100644 --- a/src/test/format/extension.format.test.ts +++ b/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([yapf, autoPep8]).then(formattedResults => { - formattedYapf = formattedResults[0]; - formattedAutoPep8 = formattedResults[1]; + const pythonProcess = await ioc.serviceContainer.get(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();