Ensure sorting imports in a modified file picks up the proper configuration

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))

src/client/common/process/proc.ts

@@ -119,6 +119,11 @@ export class ProcessService extends EventEmitter implements IProcessService {
             });
         });
 
+        if (options.input) {
+            proc.stdin.write(options.input, encoding);
+            proc.stdin.end();
+        }
+
         this.emit('exec', file, args, options);
 
         return {
@@ -163,6 +168,11 @@ export class ProcessService extends EventEmitter implements IProcessService {
             }
         });
 
+        if (options.input) {
+            proc.stdin.write(options.input, encoding);
+            proc.stdin.end();
+        }
+
         proc.once('close', () => {
             if (deferred.completed) {
                 return;

src/client/common/process/types.ts

@@ -28,6 +28,7 @@ export type ObservableExecutionResult<T extends string | Buffer> = {
 
 // tslint:disable-next-line:interface-name
 export type SpawnOptions = ChildProcessSpawnOptions & {
+    input?: string;
     encoding?: string;
     token?: CancellationToken;
     mergeStdOutErr?: boolean;

src/client/providers/importSortProvider.ts

@@ -4,7 +4,6 @@ import * as path from 'path';
 import { CancellationToken, Uri, WorkspaceEdit } from 'vscode';
 import { IApplicationShell, ICommandManager, IDocumentManager } from '../common/application/types';
 import { Commands, EXTENSION_ROOT_DIR, PYTHON_LANGUAGE, STANDARD_OUTPUT_CHANNEL } from '../common/constants';
-import { IFileSystem } from '../common/platform/types';
 import { IProcessServiceFactory, IPythonExecutionFactory } from '../common/process/types';
 import { IConfigurationService, IDisposableRegistry, IEditorUtils, ILogger, IOutputChannel } from '../common/types';
 import { noop } from '../common/utils/misc';
@@ -38,41 +37,38 @@ export class SortImportsEditingProvider implements ISortImportsEditingProvider {
         if (document.lineCount <= 1) {
             return;
         }
-        // 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 importScript = path.join(EXTENSION_ROOT_DIR, 'pythonFiles', 'sortImports.py');
-        const fsService = this.serviceContainer.get<IFileSystem>(IFileSystem);
-        const tmpFile = document.isDirty ? await fsService.createTemporaryFile(path.extname(document.uri.fsPath)) : undefined;
-        if (tmpFile) {
-            await fsService.writeFile(tmpFile.filePath, document.getText());
-        }
         const settings = this.configurationService.getSettings(uri);
         const isort = settings.sortImports.path;
-        const filePath = tmpFile ? tmpFile.filePath : document.uri.fsPath;
-        const args = [filePath, '--diff'].concat(settings.sortImports.args);
         let diffPatch: string;
 
+        // 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 direcotry to the
+        // directory which contains the file.
+        const args = ['-', '--diff'].concat(settings.sortImports.args);
+        const spawnOptions = {
+            token,
+            throwOnStdErr: true,
+            input: document.getText(),
+            cwd: path.dirname(uri.fsPath)
+        };
+
         if (token && token.isCancellationRequested) {
             return;
         }
-        try {
-            if (typeof isort === 'string' && isort.length > 0) {
-                // Lets just treat this as a standard tool.
-                const processService = await this.processServiceFactory.create(document.uri);
-                diffPatch = (await processService.exec(isort, args, { throwOnStdErr: true, token })).stdout;
-            } else {
-                const processExeService = await this.pythonExecutionFactory.create({ resource: document.uri });
-                diffPatch = (await processExeService.exec([importScript].concat(args), { throwOnStdErr: true, token })).stdout;
-            }
 
-            return this.editorUtils.getWorkspaceEditsFromPatch(document.getText(), diffPatch, document.uri);
-        } finally {
-            if (tmpFile) {
-                tmpFile.dispose();
-            }
+        if (typeof isort === 'string' && isort.length > 0) {
+            // Lets just treat this as a standard tool.
+            const processService = await this.processServiceFactory.create(document.uri);
+            diffPatch = (await processService.exec(isort, args, spawnOptions)).stdout;
+        } else {
+            const processExeService = await this.pythonExecutionFactory.create({ resource: document.uri });
+            diffPatch = (await processExeService.exec([importScript].concat(args), spawnOptions)).stdout;
         }
+
+        return this.editorUtils.getWorkspaceEditsFromPatch(document.getText(), diffPatch, document.uri);
     }
 
     public registerCommands() {

src/test/providers/importSortProvider.unit.test.ts

@@ -12,7 +12,6 @@ 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 } from '../../client/common/process/types';
 import { IConfigurationService, IDisposableRegistry, IEditorUtils, IPythonSettings, ISortImportSettings } from '../../client/common/types';
@@ -32,19 +31,16 @@ suite('Import Sort Provider', () => {
     let commandManager: TypeMoq.IMock<ICommandManager>;
     let pythonSettings: TypeMoq.IMock<IPythonSettings>;
     let sortProvider: ISortImportsEditingProvider;
-    let fs: TypeMoq.IMock<IFileSystem>;
     setup(() => {
         serviceContainer = TypeMoq.Mock.ofType<IServiceContainer>();
         commandManager = TypeMoq.Mock.ofType<ICommandManager>();
-        fs = TypeMoq.Mock.ofType<IFileSystem>();
         documentManager = TypeMoq.Mock.ofType<IDocumentManager>();
         shell = TypeMoq.Mock.ofType<IApplicationShell>();
         configurationService = TypeMoq.Mock.ofType<IConfigurationService>();
         pythonExecFactory = TypeMoq.Mock.ofType<IPythonExecutionFactory>();
         processServiceFactory = TypeMoq.Mock.ofType<IProcessServiceFactory>();
         pythonSettings = TypeMoq.Mock.ofType<IPythonSettings>();
         editorUtils = TypeMoq.Mock.ofType<IEditorUtils>();
-        fs = TypeMoq.Mock.ofType<IFileSystem>();
         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);
@@ -53,7 +49,6 @@ 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);
@@ -252,11 +247,9 @@ suite('Import Sort Provider', () => {
         shell.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<TextDocument>();
-        let tmpFileDisposed = false;
-        const tmpFile: TemporaryFile = { filePath: 'TmpFile', dispose: () => tmpFileDisposed = true };
         const processService = TypeMoq.Mock.ofType<ProcessService>();
         processService.setup((d: any) => d.then).returns(() => undefined);
         mockDoc.setup((d: any) => d.then).returns(() => undefined);
@@ -268,30 +261,24 @@ suite('Import Sort Provider', () => {
             .verifiable(TypeMoq.Times.atLeastOnce());
         mockDoc.setup(d => d.isDirty)
             .returns(() => true)
-            .verifiable(TypeMoq.Times.atLeastOnce());
+            .verifiable(TypeMoq.Times.never());
         mockDoc.setup(d => d.uri)
             .returns(() => uri)
             .verifiable(TypeMoq.Times.atLeastOnce());
         documentManager
             .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(() => { return { path: 'CUSTOM_ISORT', args: ['1', '2'] } as any as ISortImportSettings; })
             .verifiable(TypeMoq.Times.once());
         processServiceFactory.setup(p => p.create(TypeMoq.It.isAny()))
             .returns(() => Promise.resolve(processService.object))
             .verifiable(TypeMoq.Times.once());
 
-        const expectedArgs = [tmpFile.filePath, '--diff', '1', '2'];
+        const expectedArgs = ['-', '--diff', '1', '2'];
         processService
-            .setup(p => p.exec(TypeMoq.It.isValue('CUSTOM_ISORT'), TypeMoq.It.isValue(expectedArgs), TypeMoq.It.isValue({ throwOnStdErr: true, token: undefined })))
+            .setup(p => p.exec(TypeMoq.It.isValue('CUSTOM_ISORT'), TypeMoq.It.isValue(expectedArgs), TypeMoq.It.isValue({ throwOnStdErr: true, token: undefined, input: 'Hello', cwd: '/' })))
             .returns(() => Promise.resolve({ stdout: 'DIFF' }))
             .verifiable(TypeMoq.Times.once());
         const expectedEdit = new WorkspaceEdit();
@@ -303,15 +290,13 @@ 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 (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<TextDocument>();
-        let tmpFileDisposed = false;
-        const tmpFile: TemporaryFile = { filePath: 'TmpFile', dispose: () => tmpFileDisposed = true };
         const processService = TypeMoq.Mock.ofType<ProcessService>();
         processService.setup((d: any) => d.then).returns(() => undefined);
         mockDoc.setup((d: any) => d.then).returns(() => undefined);
@@ -323,20 +308,14 @@ suite('Import Sort Provider', () => {
             .verifiable(TypeMoq.Times.atLeastOnce());
         mockDoc.setup(d => d.isDirty)
             .returns(() => true)
-            .verifiable(TypeMoq.Times.atLeastOnce());
+            .verifiable(TypeMoq.Times.never());
         mockDoc.setup(d => d.uri)
             .returns(() => uri)
             .verifiable(TypeMoq.Times.atLeastOnce());
         documentManager
             .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(() => { return { args: ['1', '2'] } as any as ISortImportSettings; })
             .verifiable(TypeMoq.Times.once());
@@ -347,9 +326,9 @@ suite('Import Sort Provider', () => {
             .returns(() => Promise.resolve(processExeService.object))
             .verifiable(TypeMoq.Times.once());
         const importScript = path.join(EXTENSION_ROOT_DIR, 'pythonFiles', 'sortImports.py');
-        const expectedArgs = [importScript, tmpFile.filePath, '--diff', '1', '2'];
+        const expectedArgs = [importScript, '-', '--diff', '1', '2'];
         processExeService
-            .setup(p => p.exec(TypeMoq.It.isValue(expectedArgs), TypeMoq.It.isValue({ throwOnStdErr: true, token: undefined })))
+            .setup(p => p.exec(TypeMoq.It.isValue(expectedArgs), TypeMoq.It.isValue({ throwOnStdErr: true, token: undefined, input: 'Hello', cwd: '/' })))
             .returns(() => Promise.resolve({ stdout: 'DIFF' }))
             .verifiable(TypeMoq.Times.once());
         const expectedEdit = new WorkspaceEdit();
@@ -361,8 +340,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();
     });
 });