diff --git a/news/2 Fixes/4891.md b/news/2 Fixes/4891.md new file mode 100644 index 000000000000..bcb7b836a35e --- /dev/null +++ b/news/2 Fixes/4891.md @@ -0,0 +1,3 @@ +Ensure sorting imports in a modified file picks up the proper configuration +([#4891](https://github.com/Microsoft/vscode-python/issues/4891); +thanks [Peter Law](https://github.com/PeterJCLaw)) diff --git a/pythonFiles/sortImports.py b/pythonFiles/sortImports.py index 666827118d37..07381b915f4c 100644 --- a/pythonFiles/sortImports.py +++ b/pythonFiles/sortImports.py @@ -1,6 +1,7 @@ # Copyright (c) Microsoft Corporation. All rights reserved. # Licensed under the MIT License. +import io import os import os.path import sys @@ -8,6 +9,19 @@ isort_path = os.path.join(os.path.dirname(__file__), "lib", "python") sys.path.insert(0, isort_path) +# Work around stdin buffering issues on windows (https://bugs.python.org/issue40540) +# caused in part by isort seeking within the stdin stream by replacing the +# stream with something which is definitely seekable. +try: + # python 3 + stdin = sys.stdin.buffer +except AttributeError: + # python 2 + stdin = sys.stdin + +sys.stdin = io.BytesIO(stdin.read()) +# End workaround + import isort.main isort.main.main() diff --git a/src/client/providers/importSortProvider.ts b/src/client/providers/importSortProvider.ts index cfb5e60eaea0..63bf8dc5420c 100644 --- a/src/client/providers/importSortProvider.ts +++ b/src/client/providers/importSortProvider.ts @@ -5,38 +5,16 @@ import { CancellationToken, TextDocument, Uri, WorkspaceEdit } from 'vscode'; import { IApplicationShell, ICommandManager, IDocumentManager } from '../common/application/types'; import { Commands, PYTHON_LANGUAGE, STANDARD_OUTPUT_CHANNEL } from '../common/constants'; import { traceError } from '../common/logger'; -import { IFileSystem } from '../common/platform/types'; import * as internalScripts from '../common/process/internal/scripts'; -import { IProcessServiceFactory, IPythonExecutionFactory } from '../common/process/types'; +import { IProcessServiceFactory, IPythonExecutionFactory, ObservableExecutionResult } from '../common/process/types'; import { IConfigurationService, IDisposableRegistry, IEditorUtils, IOutputChannel } from '../common/types'; +import { createDeferred } from '../common/utils/async'; import { noop } from '../common/utils/misc'; import { IServiceContainer } from '../ioc/types'; import { captureTelemetry } from '../telemetry'; import { EventName } from '../telemetry/constants'; import { ISortImportsEditingProvider } from './types'; -async function withRealFile( - document: TextDocument, - fs: IFileSystem, - useFile: (filename: string) => Promise -): Promise<[string, T]> { - const filename = document.uri.fsPath; - const text = document.getText(); - if (document.isDirty) { - const tmpFile = await fs.createTemporaryFile(path.extname(filename)); - try { - await fs.writeFile(tmpFile.filePath, text); - const result = await useFile(tmpFile.filePath); - return [text, result]; - } finally { - tmpFile.dispose(); - } - } else { - const result = await useFile(filename); - return [text, result]; - } -} - @injectable() export class SortImportsEditingProvider implements ISortImportsEditingProvider { private readonly processServiceFactory: IProcessServiceFactory; @@ -45,6 +23,7 @@ export class SortImportsEditingProvider implements ISortImportsEditingProvider { private readonly documentManager: IDocumentManager; private readonly configurationService: IConfigurationService; private readonly editorUtils: IEditorUtils; + public constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) { this.shell = serviceContainer.get(IApplicationShell); this.documentManager = serviceContainer.get(IDocumentManager); @@ -68,20 +47,14 @@ export class SortImportsEditingProvider implements ISortImportsEditingProvider { } const execIsort = await this.getExecIsort(document, uri, token); + if (token && token.isCancellationRequested) { + return; + } + const diffPatch = await execIsort(document.getText()); - // isort does 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. less responsive solution. - const fs = this.serviceContainer.get(IFileSystem); - const [text, diffPatch] = await withRealFile(document, fs, async (filename: string) => { - if (token && token.isCancellationRequested) { - return; - } - - return execIsort(filename); - }); - return diffPatch ? this.editorUtils.getWorkspaceEditsFromPatch(text, diffPatch, document.uri) : undefined; + return diffPatch + ? this.editorUtils.getWorkspaceEditsFromPatch(document.getText(), diffPatch, document.uri) + : undefined; } public registerCommands() { @@ -129,29 +102,74 @@ export class SortImportsEditingProvider implements ISortImportsEditingProvider { } } - private async getExecIsort(document: TextDocument, uri: Uri, token?: CancellationToken) { + private async getExecIsort( + document: TextDocument, + uri: Uri, + token?: CancellationToken + ): Promise<(documentText: string) => Promise> { const settings = this.configurationService.getSettings(uri); const _isort = settings.sortImports.path; const isort = typeof _isort === 'string' && _isort.length > 0 ? _isort : undefined; const isortArgs = settings.sortImports.args; + // We pass the content of the file to be sorted via stdin. This avoids + // saving the file (as well as a potential temporary file), but does + // mean that we need another way to tell `isort` where to look for + // configuration. We do that by setting the working directory to the + // directory which contains the file. + const filename = '-'; + + const spawnOptions = { + token, + throwOnStdErr: true, + cwd: path.dirname(uri.fsPath) + }; + if (isort) { const procService = await this.processServiceFactory.create(document.uri); // Use isort directly instead of the internal script. - return async (filename: string) => { + return async (documentText: string) => { const args = getIsortArgs(filename, isortArgs); - const proc = await procService.exec(isort, args, { throwOnStdErr: true, token }); - return proc.stdout; + const result = procService.execObservable(isort, args, spawnOptions); + return this.communicateWithIsortProcess(result, documentText); }; } else { const procService = await this.pythonExecutionFactory.create({ resource: document.uri }); - return async (filename: string) => { + return async (documentText: string) => { const [args, parse] = internalScripts.sortImports(filename, isortArgs); - const proc = await procService.exec(args, { throwOnStdErr: true, token }); - return parse(proc.stdout); + const result = procService.execObservable(args, spawnOptions); + return parse(await this.communicateWithIsortProcess(result, documentText)); }; } } + + private async communicateWithIsortProcess( + observableResult: ObservableExecutionResult, + inputText: string + ): Promise { + // Configure our listening to the output from isort ... + let outputBuffer = ''; + const isortOutput = createDeferred(); + observableResult.out.subscribe({ + next: (output) => { + if (output.source === 'stdout') { + outputBuffer += output.out; + } + }, + complete: () => { + isortOutput.resolve(outputBuffer); + } + }); + + // ... then send isort the document content ... + observableResult.proc?.stdin.write(inputText); + observableResult.proc?.stdin.end(); + + // .. and finally wait for isort to do its thing + await isortOutput.promise; + + return outputBuffer; + } } function getIsortArgs(filename: string, extraArgs?: string[]): string[] { diff --git a/src/test/format/extension.sort.test.ts b/src/test/format/extension.sort.test.ts index bf266f798cca..590ba0292055 100644 --- a/src/test/format/extension.sort.test.ts +++ b/src/test/format/extension.sort.test.ts @@ -107,6 +107,7 @@ suite('Sorting', () => { const textDocument = await workspace.openTextDocument(fileToFormatWithConfig); await window.showTextDocument(textDocument); const edit = (await sorter.provideDocumentSortImportsEdits(textDocument.uri))!; + expect(edit).not.to.eq(undefined, 'No edit returned'); expect(edit.entries()).to.be.lengthOf(1); const edits = edit.entries()[0][1]; const newValue = `from third_party import lib2${EOL}from third_party import lib3${EOL}from third_party import lib4${EOL}from third_party import lib5${EOL}from third_party import lib6${EOL}from third_party import lib7${EOL}from third_party import lib8${EOL}from third_party import lib9${EOL}`; @@ -158,4 +159,18 @@ suite('Sorting', () => { await commands.executeCommand(Commands.Sort_Imports); assert.notEqual(originalContent, textDocument.getText(), 'Contents have not changed'); }); + + test('With Changes and Config implicit from cwd', async () => { + const textDocument = await workspace.openTextDocument(fileToFormatWithConfig); + assert.equal(textDocument.isDirty, false, 'Document should initially be unmodified'); + const editor = await window.showTextDocument(textDocument); + await editor.edit((builder) => { + builder.insert(new Position(0, 0), `from third_party import lib0${EOL}`); + }); + assert.equal(textDocument.isDirty, true, 'Document should have been modified (pre sort)'); + await sorter.sortImports(textDocument.uri); + assert.equal(textDocument.isDirty, true, 'Document should have been modified by sorting'); + const newValue = `from third_party import lib0${EOL}from third_party import lib1${EOL}from third_party import lib2${EOL}from third_party import lib3${EOL}from third_party import lib4${EOL}from third_party import lib5${EOL}from third_party import lib6${EOL}from third_party import lib7${EOL}from third_party import lib8${EOL}from third_party import lib9${EOL}`; + assert.equal(textDocument.getText(), newValue); + }); }); diff --git a/src/test/providers/importSortProvider.unit.test.ts b/src/test/providers/importSortProvider.unit.test.ts index 1078a8b58e5c..6d58a81f44b4 100644 --- a/src/test/providers/importSortProvider.unit.test.ts +++ b/src/test/providers/importSortProvider.unit.test.ts @@ -6,18 +6,22 @@ // tslint:disable:no-any max-func-body-length import { expect } from 'chai'; +import { ChildProcess } from 'child_process'; import { EOL } from 'os'; import * as path from 'path'; +import { Observable } from 'rxjs/Observable'; +import { Subscriber } from 'rxjs/Subscriber'; +import { Writable } from 'stream'; import * as TypeMoq from 'typemoq'; import { Range, TextDocument, TextEditor, TextLine, Uri, WorkspaceEdit } from 'vscode'; import { IApplicationShell, ICommandManager, IDocumentManager } from '../../client/common/application/types'; import { Commands, EXTENSION_ROOT_DIR } from '../../client/common/constants'; -import { IFileSystem, TemporaryFile } from '../../client/common/platform/types'; import { ProcessService } from '../../client/common/process/proc'; import { IProcessServiceFactory, IPythonExecutionFactory, - IPythonExecutionService + IPythonExecutionService, + Output } from '../../client/common/process/types'; import { IConfigurationService, @@ -44,11 +48,9 @@ suite('Import Sort Provider', () => { let commandManager: TypeMoq.IMock; let pythonSettings: TypeMoq.IMock; let sortProvider: ISortImportsEditingProvider; - let fs: TypeMoq.IMock; setup(() => { serviceContainer = TypeMoq.Mock.ofType(); commandManager = TypeMoq.Mock.ofType(); - fs = TypeMoq.Mock.ofType(); documentManager = TypeMoq.Mock.ofType(); shell = TypeMoq.Mock.ofType(); configurationService = TypeMoq.Mock.ofType(); @@ -56,7 +58,6 @@ suite('Import Sort Provider', () => { processServiceFactory = TypeMoq.Mock.ofType(); pythonSettings = TypeMoq.Mock.ofType(); editorUtils = TypeMoq.Mock.ofType(); - fs = TypeMoq.Mock.ofType(); serviceContainer.setup((c) => c.get(ICommandManager)).returns(() => commandManager.object); serviceContainer.setup((c) => c.get(IDocumentManager)).returns(() => documentManager.object); serviceContainer.setup((c) => c.get(IApplicationShell)).returns(() => shell.object); @@ -65,9 +66,7 @@ suite('Import Sort Provider', () => { serviceContainer.setup((c) => c.get(IProcessServiceFactory)).returns(() => processServiceFactory.object); serviceContainer.setup((c) => c.get(IEditorUtils)).returns(() => editorUtils.object); serviceContainer.setup((c) => c.get(IDisposableRegistry)).returns(() => []); - serviceContainer.setup((c) => c.get(IFileSystem)).returns(() => fs.object); configurationService.setup((c) => c.getSettings(TypeMoq.It.isAny())).returns(() => pythonSettings.object); - sortProvider = new SortImportsEditingProvider(serviceContainer.object); }); @@ -279,11 +278,9 @@ suite('Import Sort Provider', () => { shell.verifyAll(); documentManager.verifyAll(); }); - test('Ensure temporary file is created for sorting when document is dirty (with custom isort path)', async () => { + test('Ensure stdin is used for sorting (with custom isort path)', async () => { const uri = Uri.file('something.py'); const mockDoc = TypeMoq.Mock.ofType(); - let tmpFileDisposed = false; - const tmpFile: TemporaryFile = { filePath: 'TmpFile', dispose: () => (tmpFileDisposed = true) }; const processService = TypeMoq.Mock.ofType(); processService.setup((d: any) => d.then).returns(() => undefined); mockDoc.setup((d: any) => d.then).returns(() => undefined); @@ -298,7 +295,7 @@ suite('Import Sort Provider', () => { mockDoc .setup((d) => d.isDirty) .returns(() => true) - .verifiable(TypeMoq.Times.atLeastOnce()); + .verifiable(TypeMoq.Times.never()); mockDoc .setup((d) => d.uri) .returns(() => uri) @@ -307,12 +304,6 @@ suite('Import Sort Provider', () => { .setup((d) => d.openTextDocument(TypeMoq.It.isValue(uri))) .returns(() => Promise.resolve(mockDoc.object)) .verifiable(TypeMoq.Times.atLeastOnce()); - fs.setup((f) => f.createTemporaryFile(TypeMoq.It.isValue('.py'))) - .returns(() => Promise.resolve(tmpFile)) - .verifiable(TypeMoq.Times.once()); - fs.setup((f) => f.writeFile(TypeMoq.It.isValue(tmpFile.filePath), TypeMoq.It.isValue('Hello'))) - .returns(() => Promise.resolve(undefined)) - .verifiable(TypeMoq.Times.once()); pythonSettings .setup((s) => s.sortImports) .returns(() => { @@ -324,16 +315,33 @@ suite('Import Sort Provider', () => { .returns(() => Promise.resolve(processService.object)) .verifiable(TypeMoq.Times.once()); - const expectedArgs = [tmpFile.filePath, '--diff', '1', '2']; + let actualSubscriber: Subscriber>; + const stdinStream = TypeMoq.Mock.ofType(); + stdinStream.setup((s) => s.write('Hello')).verifiable(TypeMoq.Times.once()); + stdinStream + .setup((s) => s.end()) + .callback(() => { + actualSubscriber.next({ source: 'stdout', out: 'DIFF' }); + actualSubscriber.complete(); + }) + .verifiable(TypeMoq.Times.once()); + const childProcess = TypeMoq.Mock.ofType(); + childProcess.setup((p) => p.stdin).returns(() => stdinStream.object); + const executionResult = { + proc: childProcess.object, + out: new Observable>((subscriber) => (actualSubscriber = subscriber)), + dispose: noop + }; + const expectedArgs = ['-', '--diff', '1', '2']; processService .setup((p) => - p.exec( + p.execObservable( TypeMoq.It.isValue('CUSTOM_ISORT'), TypeMoq.It.isValue(expectedArgs), - TypeMoq.It.isValue({ throwOnStdErr: true, token: undefined }) + TypeMoq.It.isValue({ throwOnStdErr: true, token: undefined, cwd: path.sep }) ) ) - .returns(() => Promise.resolve({ stdout: 'DIFF' })) + .returns(() => executionResult) .verifiable(TypeMoq.Times.once()); const expectedEdit = new WorkspaceEdit(); editorUtils @@ -350,15 +358,15 @@ suite('Import Sort Provider', () => { const edit = await sortProvider.provideDocumentSortImportsEdits(uri); expect(edit).to.be.equal(expectedEdit); - expect(tmpFileDisposed).to.be.equal(true, 'Temporary file not disposed'); shell.verifyAll(); + mockDoc.verifyAll(); documentManager.verifyAll(); }); - test('Ensure temporary file is created for sorting when document is dirty', async () => { + test('Ensure stdin is used for sorting', async () => { const uri = Uri.file('something.py'); const mockDoc = TypeMoq.Mock.ofType(); - let tmpFileDisposed = false; - const tmpFile: TemporaryFile = { filePath: 'TmpFile', dispose: () => (tmpFileDisposed = true) }; + const processService = TypeMoq.Mock.ofType(); + processService.setup((d: any) => d.then).returns(() => undefined); mockDoc.setup((d: any) => d.then).returns(() => undefined); mockDoc .setup((d) => d.lineCount) @@ -371,7 +379,7 @@ suite('Import Sort Provider', () => { mockDoc .setup((d) => d.isDirty) .returns(() => true) - .verifiable(TypeMoq.Times.atLeastOnce()); + .verifiable(TypeMoq.Times.never()); mockDoc .setup((d) => d.uri) .returns(() => uri) @@ -380,12 +388,6 @@ suite('Import Sort Provider', () => { .setup((d) => d.openTextDocument(TypeMoq.It.isValue(uri))) .returns(() => Promise.resolve(mockDoc.object)) .verifiable(TypeMoq.Times.atLeastOnce()); - fs.setup((f) => f.createTemporaryFile(TypeMoq.It.isValue('.py'))) - .returns(() => Promise.resolve(tmpFile)) - .verifiable(TypeMoq.Times.once()); - fs.setup((f) => f.writeFile(TypeMoq.It.isValue(tmpFile.filePath), TypeMoq.It.isValue('Hello'))) - .returns(() => Promise.resolve(undefined)) - .verifiable(TypeMoq.Times.once()); pythonSettings .setup((s) => s.sortImports) .returns(() => { @@ -399,13 +401,34 @@ suite('Import Sort Provider', () => { .setup((p) => p.create(TypeMoq.It.isAny())) .returns(() => Promise.resolve(processExeService.object)) .verifiable(TypeMoq.Times.once()); + + let actualSubscriber: Subscriber>; + const stdinStream = TypeMoq.Mock.ofType(); + stdinStream.setup((s) => s.write('Hello')).verifiable(TypeMoq.Times.once()); + stdinStream + .setup((s) => s.end()) + .callback(() => { + actualSubscriber.next({ source: 'stdout', out: 'DIFF' }); + actualSubscriber.complete(); + }) + .verifiable(TypeMoq.Times.once()); + const childProcess = TypeMoq.Mock.ofType(); + childProcess.setup((p) => p.stdin).returns(() => stdinStream.object); + const executionResult = { + proc: childProcess.object, + out: new Observable>((subscriber) => (actualSubscriber = subscriber)), + dispose: noop + }; const importScript = path.join(EXTENSION_ROOT_DIR, 'pythonFiles', 'sortImports.py'); - const expectedArgs = [ISOLATED, importScript, tmpFile.filePath, '--diff', '1', '2']; + const expectedArgs = [ISOLATED, importScript, '-', '--diff', '1', '2']; processExeService .setup((p) => - p.exec(TypeMoq.It.isValue(expectedArgs), TypeMoq.It.isValue({ throwOnStdErr: true, token: undefined })) + p.execObservable( + TypeMoq.It.isValue(expectedArgs), + TypeMoq.It.isValue({ throwOnStdErr: true, token: undefined, cwd: path.sep }) + ) ) - .returns(() => Promise.resolve({ stdout: 'DIFF' })) + .returns(() => executionResult) .verifiable(TypeMoq.Times.once()); const expectedEdit = new WorkspaceEdit(); editorUtils @@ -422,8 +445,8 @@ suite('Import Sort Provider', () => { const edit = await sortProvider.provideDocumentSortImportsEdits(uri); expect(edit).to.be.equal(expectedEdit); - expect(tmpFileDisposed).to.be.equal(true, 'Temporary file not disposed'); shell.verifyAll(); + mockDoc.verifyAll(); documentManager.verifyAll(); }); });