Ensure sorting imports in a modified file picks up the proper configuration (#9128)

PeterJCLaw · web-flow · commit 3e63746e4a62 · 2020-05-26T07:49:29.000-07:00
* Add support for passing stdin content to an exec'd process * Use stdin to pass Python source to isort This ensures that isort behaves consistently whether or not the file has modifications or has even been saved. * Add a news entry * Make these tests Windows compatible (hopefully) * Placate prettier * Test passing input to a launched STDIN * Document SpawnOptions.input * Clarify that the input stream will be closed if using SpawnOptions.input * Fix typo * Switch to handling input writing directly in the isort provider This changes from using .exec(...) to using .execObservable(...) and then interacting with the process more directly. * Remove now unused support for exec{,Observable} writing to stdin * Add logging to hopefully elucidate more information from Windows CI * Work around issues with isort seeking within stdin on Windows See comment for details. * System test for import sorting of modified files This validates that the configuration is picked up for these files. * Remove verbose logging lines from d92ceef This mostly reverts d92ceef, except for the extra test assertion it added which is still possibly useful.
diff --git a/news/2 Fixes/4891.md 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
@@ -1,13 +1,27 @@
 # Copyright (c) Microsoft Corporation. All rights reserved.
 # Licensed under the MIT License.
 
+import io
 import os
 import os.path
 import sys
 
 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
@@ -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<T>(
-    document: TextDocument,
-    fs: IFileSystem,
-    useFile: (filename: string) => Promise<T>
-): 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>(IApplicationShell);
         this.documentManager = serviceContainer.get<IDocumentManager>(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>(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<string>> {
         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<string>,
+        inputText: string
+    ): Promise<string> {
+        // Configure our listening to the output from isort ...
+        let outputBuffer = '';
+        const isortOutput = createDeferred<string>();
+        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
@@ -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

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+Ensure sorting imports in a modified file picks up the proper configuration`
	`2`	`+([#4891](https://github.com/Microsoft/vscode-python/issues/4891);`
	`3`	`+thanks [Peter Law](https://github.com/PeterJCLaw))`