#730, #838 (#845)

* Fix pylint search * Handle quote escapes in strings * Escapes in strings * CR feedback * Missing pip * Test * Tests * Tests * Mac python path * Tests * Tests * Test * "Go To Python object" doesn't work * Proper detection and type population in virtual env * Test fixes * Simplify venv check * Remove duplicates * Test * Discover pylintrc better + tests * Undo change * CR feedback * Set interprereter before checking install * Fix typo and path compare on Windows * Rename method * #815 - 'F' in flake8 means warning * 730 - same folder temp * Properly resolve ~ * Test * Test
microsoft · Feb 21, 2018 · b5d59ff · b5d59ff
1 parent 02dc3ed
commit b5d59ff
Show file tree

Hide file tree

Showing 8 changed files with 106 additions and 33 deletions.
diff --git a/package.json b/package.json
@@ -1859,4 +1859,4 @@
         "publisherDisplayName": "Microsoft",
         "publisherId": "998b010b-e2af-44a5-a6cd-0b5fd3b9b6f8"
     }
-}
+}
diff --git a/src/client/common/editor.ts b/src/client/common/editor.ts
@@ -1,5 +1,6 @@
 import * as dmp from 'diff-match-patch';
 import * as fs from 'fs';
+import * as md5 from 'md5';
 import { EOL } from 'os';
 import * as path from 'path';
 import { Position, Range, TextDocument, TextEdit, WorkspaceEdit } from 'vscode';
@@ -220,18 +221,19 @@ function getTextEditsInternal(before: string, diffs: [number, string][], startLi
 export function getTempFileWithDocumentContents(document: TextDocument): Promise<string> {
     return new Promise<string>((resolve, reject) => {
         const ext = path.extname(document.uri.fsPath);
-        // tslint:disable-next-line:no-shadowed-variable no-require-imports
-        const tmp = require('tmp');
-        tmp.file({ postfix: ext }, (err, tmpFilePath, fd) => {
-            if (err) {
-                return reject(err);
+        // Don't create file in temp folder since external utilities
+        // look into configuration files in the workspace and are not able
+        // to find custom rules if file is saved in a random disk location.
+        // This means temp file has to be created in the same folder
+        // as the original one and then removed.
+
+        // tslint:disable-next-line:no-require-imports
+        const fileName = `${document.uri.fsPath}.${md5(document.uri.fsPath)}${ext}`;
+        fs.writeFile(fileName, document.getText(), ex => {
+            if (ex) {
+                reject(`Failed to create a temporary file, ${ex.message}`);
             }
-            fs.writeFile(tmpFilePath, document.getText(), ex => {
-                if (ex) {
-                    return reject(`Failed to create a temporary file, ${ex.message}`);
-                }
-                resolve(tmpFilePath);
-            });
+            resolve(fileName);
         });
     });
 }

diff --git a/src/client/formatters/baseFormatter.ts b/src/client/formatters/baseFormatter.ts
@@ -4,6 +4,7 @@ import * as vscode from 'vscode';
 import { OutputChannel, TextEdit, Uri } from 'vscode';
 import { IWorkspaceService } from '../common/application/types';
 import { STANDARD_OUTPUT_CHANNEL } from '../common/constants';
+import '../common/extensions';
 import { isNotInstalledError } from '../common/helpers';
 import { IPythonToolExecutionService } from '../common/process/types';
 import { IInstaller, IOutputChannel, Product } from '../common/types';
@@ -50,37 +51,32 @@ export abstract class BaseFormatter {
         // 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.
-        const tmpFileCreated = document.isDirty;
-        const filePromise = tmpFileCreated ? getTempFileWithDocumentContents(document) : Promise.resolve(document.fileName);
-        const filePath = await filePromise;
-        if (token && token.isCancellationRequested) {
+        const tempFile = await this.createTempFile(document);
+        if (this.checkCancellation(document.fileName, tempFile, token)) {
             return [];
         }
 
         const executionInfo = this.helper.getExecutionInfo(this.product, args, document.uri);
-        executionInfo.args.push(filePath);
+        executionInfo.args.push(tempFile);
         const pythonToolsExecutionService = this.serviceContainer.get<IPythonToolExecutionService>(IPythonToolExecutionService);
         const promise = pythonToolsExecutionService.exec(executionInfo, { cwd, throwOnStdErr: true, token }, document.uri)
             .then(output => output.stdout)
             .then(data => {
-                if (token && token.isCancellationRequested) {
+                if (this.checkCancellation(document.fileName, tempFile, token)) {
                     return [] as TextEdit[];
                 }
                 return getTextEditsFromPatch(document.getText(), data);
             })
             .catch(error => {
-                if (token && token.isCancellationRequested) {
+                if (this.checkCancellation(document.fileName, tempFile, token)) {
                     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.unlinkSync(filePath);
-                }
+                this.deleteTempFile(document.fileName, tempFile).ignoreErrors();
                 return edits;
             });
         vscode.window.setStatusBarMessage(`Formatting with ${this.Id}`, promise);
@@ -101,4 +97,25 @@ export abstract class BaseFormatter {
 
         this.outputChannel.appendLine(`\n${customError}\n${error}`);
     }
+
+    private async createTempFile(document: vscode.TextDocument): Promise<string> {
+        return document.isDirty
+            ? await getTempFileWithDocumentContents(document)
+            : document.fileName;
+    }
+
+    private deleteTempFile(originalFile: string, tempFile: string): Promise<void> {
+        if (originalFile !== tempFile) {
+            return fs.unlink(tempFile);
+        }
+        return Promise.resolve();
+    }
+
+    private checkCancellation(originalFile: string, tempFile: string, token?: vscode.CancellationToken): boolean {
+        if (token && token.isCancellationRequested) {
+            this.deleteTempFile(originalFile, tempFile).ignoreErrors();
+            return true;
+        }
+        return false;
+    }
 }
diff --git a/src/client/linters/pylint.ts b/src/client/linters/pylint.ts
@@ -2,6 +2,7 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License.
 
+import * as os from 'os';
 import * as path from 'path';
 import { OutputChannel } from 'vscode';
 import { CancellationToken, TextDocument } from 'vscode';
@@ -76,13 +77,12 @@ export class Pylint extends BaseLinter {
             return true;
         }
 
-        let dir = folder;
-        if (await fs.fileExistsAsync(path.join(dir, pylintrc)) || await fs.fileExistsAsync(path.join(dir, dotPylintrc))) {
+        if (await fs.fileExistsAsync(path.join(folder, pylintrc)) || await fs.fileExistsAsync(path.join(folder, dotPylintrc))) {
             return true;
         }
 
-        let current = dir;
-        let above = path.dirname(dir);
+        let current = folder;
+        let above = path.dirname(folder);
         do {
             if (!await fs.fileExistsAsync(path.join(current, '__init__.py'))) {
                 break;
@@ -94,11 +94,11 @@ export class Pylint extends BaseLinter {
             above = path.dirname(above);
         } while (!fs.arePathsSame(current, above));
 
-        dir = path.resolve('~');
-        if (await fs.fileExistsAsync(path.join(dir, dotPylintrc))) {
+        const home = os.homedir();
+        if (await fs.fileExistsAsync(path.join(home, dotPylintrc))) {
             return true;
         }
-        if (await fs.fileExistsAsync(path.join(dir, '.config', pylintrc))) {
+        if (await fs.fileExistsAsync(path.join(home, '.config', pylintrc))) {
             return true;
         }
 

diff --git a/src/test/format/extension.format.test.ts b/src/test/format/extension.format.test.ts
@@ -23,6 +23,7 @@ const yapfFileToAutoFormat = path.join(formatFilesPath, 'yapfFileToAutoFormat.py
 let formattedYapf = '';
 let formattedAutoPep8 = '';
 
+// tslint:disable-next-line:max-func-body-length
 suite('Formatting', () => {
     let ioc: UnitTestIocContainer;
 
@@ -94,7 +95,53 @@ suite('Formatting', () => {
         });
         compareFiles(formattedContents, textEditor.document.getText());
     }
-    test('AutoPep8', async () => await testFormatting(new AutoPep8Formatter(ioc.serviceContainer), formattedAutoPep8, autoPep8FileToFormat, 'autopep8.output'));
 
+    test('AutoPep8', async () => await testFormatting(new AutoPep8Formatter(ioc.serviceContainer), formattedAutoPep8, autoPep8FileToFormat, 'autopep8.output'));
     test('Yapf', async () => await testFormatting(new YapfFormatter(ioc.serviceContainer), formattedYapf, yapfFileToFormat, 'yapf.output'));
+
+    test('Yapf on dirty file', async () => {
+        const sourceDir = path.join(__dirname, '..', '..', '..', 'src', 'test', 'pythonFiles', 'formatting');
+        const targetDir = path.join(__dirname, '..', 'pythonFiles', 'formatting');
+
+        const originalName = 'formatWhenDirty.py';
+        const resultsName = 'formatWhenDirtyResult.py';
+        const fileToFormat = path.join(targetDir, originalName);
+        const formattedFile = path.join(targetDir, resultsName);
+
+        if (!fs.pathExistsSync(targetDir)) {
+            fs.mkdirSync(targetDir);
+        }
+        fs.copySync(path.join(sourceDir, originalName), fileToFormat, { overwrite: true });
+        fs.copySync(path.join(sourceDir, resultsName), formattedFile, { overwrite: true });
+
+        const textDocument = await vscode.workspace.openTextDocument(fileToFormat);
+        const textEditor = await vscode.window.showTextDocument(textDocument);
+        await textEditor.edit(builder => {
+            // Make file dirty. Trailing blanks will be removed.
+            builder.insert(new vscode.Position(0, 0), '\n    \n');
+        });
+
+        const dir = path.dirname(fileToFormat);
+        const configFile = path.join(dir, '.style.yapf');
+        try {
+            // Create yapf configuration file
+            const content = '[style]\nbased_on_style = pep8\nindent_width=5\n';
+            fs.writeFileSync(configFile, content);
+
+            const options = { insertSpaces: textEditor.options.insertSpaces! as boolean, tabSize: 1 };
+            const formatter = new YapfFormatter(ioc.serviceContainer);
+            const edits = await formatter.formatDocument(textDocument, options, new CancellationTokenSource().token);
+            await textEditor.edit(editBuilder => {
+                edits.forEach(edit => editBuilder.replace(edit.range, edit.newText));
+            });
+
+            const expected = fs.readFileSync(formattedFile).toString();
+            const actual = textEditor.document.getText();
+            compareFiles(expected, actual);
+        } finally {
+            if (fs.existsSync(configFile)) {
+                fs.unlinkSync(configFile);
+            }
+        }
+    });
 });
diff --git a/src/test/linters/pylint.test.ts b/src/test/linters/pylint.test.ts
@@ -3,6 +3,7 @@
 
 import { expect } from 'chai';
 import { Container } from 'inversify';
+import * as os from 'os';
 import * as path from 'path';
 import * as TypeMoq from 'typemoq';
 import { CancellationTokenSource, OutputChannel, TextDocument, Uri, WorkspaceFolder } from 'vscode';
@@ -100,15 +101,15 @@ suite('Linting - Pylintrc search', () => {
         expect(result).to.be.equal(true, `'${dotPylintrc}' not detected in the module tree.`);
     });
     test('.pylintrc up the ~ folder', async () => {
-        const home = path.resolve('~');
+        const home = os.homedir();
         const rc = path.join(home, dotPylintrc);
         fileSystem.setup(x => x.fileExistsAsync(rc)).returns(() => Promise.resolve(true));
 
         const result = await Pylint.hasConfigurationFile(fileSystem.object, basePath, platformService.object);
         expect(result).to.be.equal(true, `'${dotPylintrc}' not detected in the ~ folder.`);
     });
     test('pylintrc up the ~/.config folder', async () => {
-        const home = path.resolve('~');
+        const home = os.homedir();
         const rc = path.join(home, '.config', pylintrc);
         fileSystem.setup(x => x.fileExistsAsync(rc)).returns(() => Promise.resolve(true));
 

diff --git a/src/test/pythonFiles/formatting/formatWhenDirty.py b/src/test/pythonFiles/formatting/formatWhenDirty.py
@@ -0,0 +1,3 @@
+x = 0
+if x > 0:
+  x = 1
diff --git a/src/test/pythonFiles/formatting/formatWhenDirtyResult.py b/src/test/pythonFiles/formatting/formatWhenDirtyResult.py
@@ -0,0 +1,3 @@
+x = 0
+if x > 0:
+     x = 1