From 654689286183a40ec7d78bdb5e303f97af23d5c3 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Fri, 16 Feb 2018 10:50:36 -0800 Subject: [PATCH 01/32] Fix pylint search --- src/client/linters/pylint.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/linters/pylint.ts b/src/client/linters/pylint.ts index c650714e45dd..6b7fb6dfc826 100644 --- a/src/client/linters/pylint.ts +++ b/src/client/linters/pylint.ts @@ -30,7 +30,7 @@ export class Pylint extends BaseLinter { const settings = this.configService.getSettings(uri); if (settings.linting.pylintUseMinimalCheckers && this.info.linterArgs(uri).length === 0 - && !await Pylint.hasConfigurationFile(this.fileSystem, uri.fsPath, this.platformService)) { + && !await Pylint.hasConfigurationFile(this.fileSystem, this.getWorkspaceRootPath(document), this.platformService)) { minArgs = [ '--disable=all', '--enable=F,E,unreachable,duplicate-key,unnecessary-semicolon,global-variable-not-assigned,unused-variable,unused-wildcard-import,binary-op-exception,bad-format-string,anomalous-backslash-in-string,bad-open-mode' From 76af122d7ede6cf8947df0bd99b1466d96c0a10d Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Fri, 16 Feb 2018 12:41:31 -0800 Subject: [PATCH 02/32] Handle quote escapes in strings --- src/client/language/tokenizer.ts | 5 ++++- src/test/language/tokenizer.test.ts | 15 +++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/src/client/language/tokenizer.ts b/src/client/language/tokenizer.ts index bf20fb3f44ba..07c6717cbe10 100644 --- a/src/client/language/tokenizer.ts +++ b/src/client/language/tokenizer.ts @@ -359,7 +359,10 @@ export class Tokenizer implements ITokenizer { } private skipToSingleEndQuote(quote: number): void { - while (!this.cs.isEndOfStream() && this.cs.currentChar !== quote) { + while (!this.cs.isEndOfStream()) { + if (this.cs.currentChar === quote && this.cs.prevChar !== Char.Backslash) { + break; + } this.cs.moveNext(); } this.cs.moveNext(); diff --git a/src/test/language/tokenizer.test.ts b/src/test/language/tokenizer.test.ts index c397ffec95e5..e11df6a147b0 100644 --- a/src/test/language/tokenizer.test.ts +++ b/src/test/language/tokenizer.test.ts @@ -64,6 +64,21 @@ suite('Language.Tokenizer', () => { assert.equal(tokens.getItemAt(i).type, TokenType.String); } }); + test('Strings: single quote escape', async () => { + const t = new Tokenizer(); + // tslint:disable-next-line:quotemark + const tokens = t.tokenize("'\\'quoted\\''"); + assert.equal(tokens.count, 1); + assert.equal(tokens.getItemAt(0).type, TokenType.String); + assert.equal(tokens.getItemAt(0).length, 12); + }); + test('Strings: double quote escape', async () => { + const t = new Tokenizer(); + const tokens = t.tokenize('"\\"quoted\\""'); + assert.equal(tokens.count, 1); + assert.equal(tokens.getItemAt(0).type, TokenType.String); + assert.equal(tokens.getItemAt(0).length, 12); + }); test('Comments', async () => { const t = new Tokenizer(); const tokens = t.tokenize(' #co"""mment1\n\t\n#comm\'ent2 '); From 5d4d022825f4a3f4217ea73b49d033b3ee42f174 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Fri, 16 Feb 2018 12:52:05 -0800 Subject: [PATCH 03/32] Escapes in strings --- src/client/language/tokenizer.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/client/language/tokenizer.ts b/src/client/language/tokenizer.ts index 07c6717cbe10..1a4d800fed0c 100644 --- a/src/client/language/tokenizer.ts +++ b/src/client/language/tokenizer.ts @@ -360,7 +360,11 @@ export class Tokenizer implements ITokenizer { private skipToSingleEndQuote(quote: number): void { while (!this.cs.isEndOfStream()) { - if (this.cs.currentChar === quote && this.cs.prevChar !== Char.Backslash) { + if (this.cs.currentChar === Char.Backslash && this.cs.nextChar === quote) { + this.cs.advance(2); + continue; + } + if (this.cs.currentChar === quote) { break; } this.cs.moveNext(); From 29edac2d9063d6ee04ba16f4418a5ae8b79ce6c4 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Fri, 16 Feb 2018 14:02:20 -0800 Subject: [PATCH 04/32] CR feedback --- src/client/linters/pylint.ts | 9 ++++++--- src/test/linters/pylint.test.ts | 15 +++++++-------- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/src/client/linters/pylint.ts b/src/client/linters/pylint.ts index 6b7fb6dfc826..c6f6e466ba10 100644 --- a/src/client/linters/pylint.ts +++ b/src/client/linters/pylint.ts @@ -25,11 +25,14 @@ export class Pylint extends BaseLinter { let minArgs: string[] = []; // Only use minimal checkers if // a) there are no custom arguments and - // b) there is no pylintrc file + // b) there is no pylintrc file next to the file or at the workspace root const uri = document.uri; const settings = this.configService.getSettings(uri); if (settings.linting.pylintUseMinimalCheckers && this.info.linterArgs(uri).length === 0 + // Check pylintrc next to the file + && !await Pylint.hasConfigurationFile(this.fileSystem, path.dirname(uri.fsPath), this.platformService) + // Checn for pylintrc at the root (function will strip the file name) && !await Pylint.hasConfigurationFile(this.fileSystem, this.getWorkspaceRootPath(document), this.platformService)) { minArgs = [ '--disable=all', @@ -51,7 +54,7 @@ export class Pylint extends BaseLinter { } // tslint:disable-next-line:member-ordering - public static async hasConfigurationFile(fs: IFileSystem, filePath: string, platformService: IPlatformService): Promise { + public static async hasConfigurationFile(fs: IFileSystem, folder: string, platformService: IPlatformService): Promise { // https://pylint.readthedocs.io/en/latest/user_guide/run.html // https://github.com/PyCQA/pylint/blob/975e08148c0faa79958b459303c47be1a2e1500a/pylint/config.py // 1. pylintrc in the current working directory @@ -69,7 +72,7 @@ export class Pylint extends BaseLinter { return true; } - let dir = path.dirname(filePath); + let dir = folder; const pylintrc = 'pylintrc'; const dotPylintrc = '.pylintrc'; if (await fs.fileExistsAsync(path.join(dir, pylintrc)) || await fs.fileExistsAsync(path.join(dir, dotPylintrc))) { diff --git a/src/test/linters/pylint.test.ts b/src/test/linters/pylint.test.ts index cc527e320478..5ed565b2d44f 100644 --- a/src/test/linters/pylint.test.ts +++ b/src/test/linters/pylint.test.ts @@ -9,7 +9,6 @@ import { Pylint } from '../../client/linters/pylint'; suite('Linting - Pylintrc search', () => { const basePath = '/user/a/b/c/d'; - const file = path.join(basePath, 'file.py'); const pylintrc = 'pylintrc'; const dotPylintrc = '.pylintrc'; @@ -23,11 +22,11 @@ suite('Linting - Pylintrc search', () => { test('pylintrc in the file folder', async () => { fileSystem.setup(x => x.fileExistsAsync(path.join(basePath, pylintrc))).returns(() => Promise.resolve(true)); - let result = await Pylint.hasConfigurationFile(fileSystem.object, file, platformService.object); + let result = await Pylint.hasConfigurationFile(fileSystem.object, basePath, platformService.object); expect(result).to.be.equal(true, `'${pylintrc}' not detected in the file folder.`); fileSystem.setup(x => x.fileExistsAsync(path.join(basePath, dotPylintrc))).returns(() => Promise.resolve(true)); - result = await Pylint.hasConfigurationFile(fileSystem.object, file, platformService.object); + result = await Pylint.hasConfigurationFile(fileSystem.object, basePath, platformService.object); expect(result).to.be.equal(true, `'${dotPylintrc}' not detected in the file folder.`); }); test('pylintrc up the module tree', async () => { @@ -41,7 +40,7 @@ suite('Linting - Pylintrc search', () => { fileSystem.setup(x => x.fileExistsAsync(module3)).returns(() => Promise.resolve(true)); fileSystem.setup(x => x.fileExistsAsync(rc)).returns(() => Promise.resolve(true)); - const result = await Pylint.hasConfigurationFile(fileSystem.object, file, platformService.object); + const result = await Pylint.hasConfigurationFile(fileSystem.object, basePath, platformService.object); expect(result).to.be.equal(true, `'${pylintrc}' not detected in the module tree.`); }); test('.pylintrc up the module tree', async () => { @@ -56,7 +55,7 @@ suite('Linting - Pylintrc search', () => { fileSystem.setup(x => x.fileExistsAsync(module3)).returns(() => Promise.resolve(true)); fileSystem.setup(x => x.fileExistsAsync(rc)).returns(() => Promise.resolve(true)); - const result = await Pylint.hasConfigurationFile(fileSystem.object, file, platformService.object); + const result = await Pylint.hasConfigurationFile(fileSystem.object, basePath, platformService.object); expect(result).to.be.equal(true, `'${dotPylintrc}' not detected in the module tree.`); }); test('.pylintrc up the ~ folder', async () => { @@ -64,7 +63,7 @@ suite('Linting - Pylintrc search', () => { const rc = path.join(home, dotPylintrc); fileSystem.setup(x => x.fileExistsAsync(rc)).returns(() => Promise.resolve(true)); - const result = await Pylint.hasConfigurationFile(fileSystem.object, file, platformService.object); + 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 () => { @@ -72,7 +71,7 @@ suite('Linting - Pylintrc search', () => { const rc = path.join(home, '.config', pylintrc); fileSystem.setup(x => x.fileExistsAsync(rc)).returns(() => Promise.resolve(true)); - const result = await Pylint.hasConfigurationFile(fileSystem.object, file, platformService.object); + const result = await Pylint.hasConfigurationFile(fileSystem.object, basePath, platformService.object); expect(result).to.be.equal(true, `'${pylintrc}' not detected in the ~/.config folder.`); }); test('pylintrc in the /etc folder', async () => { @@ -80,7 +79,7 @@ suite('Linting - Pylintrc search', () => { const rc = path.join('/etc', pylintrc); fileSystem.setup(x => x.fileExistsAsync(rc)).returns(() => Promise.resolve(true)); - const result = await Pylint.hasConfigurationFile(fileSystem.object, file, platformService.object); + const result = await Pylint.hasConfigurationFile(fileSystem.object, basePath, platformService.object); expect(result).to.be.equal(true, `'${pylintrc}' not detected in the /etc folder.`); }); }); From 0492aabc644e64d01ddd76cf018966601eb1f157 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Fri, 16 Feb 2018 16:25:50 -0800 Subject: [PATCH 05/32] Missing pip --- src/client/common/installer/channelManager.ts | 69 ++++++++++ src/client/common/installer/condaInstaller.ts | 8 +- src/client/common/installer/installer.ts | 35 +---- .../common/installer/productInstaller.ts | 51 +------ src/client/common/installer/productNames.ts | 19 +++ .../common/installer/pythonInstallation.ts | 5 +- .../common/installer/serviceRegistry.ts | 4 +- src/client/common/installer/types.ts | 8 ++ src/client/interpreter/locators/helpers.ts | 4 + .../services/baseVirtualEnvService.ts | 25 ++-- src/test/initialize.ts | 2 +- src/test/install/channelManager.test.ts | 127 ++++++++++++++++++ 12 files changed, 257 insertions(+), 100 deletions(-) create mode 100644 src/client/common/installer/channelManager.ts create mode 100644 src/client/common/installer/productNames.ts create mode 100644 src/test/install/channelManager.test.ts diff --git a/src/client/common/installer/channelManager.ts b/src/client/common/installer/channelManager.ts new file mode 100644 index 000000000000..252f467319e8 --- /dev/null +++ b/src/client/common/installer/channelManager.ts @@ -0,0 +1,69 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { inject, injectable } from 'inversify'; +import { QuickPickItem, Uri } from 'vscode'; +import { IInterpreterService, InterpreterType } from '../../interpreter/contracts'; +import { IServiceContainer } from '../../ioc/types'; +import { IApplicationShell } from '../application/types'; +import { IPlatformService } from '../platform/types'; +import { Product } from '../types'; +import { ProductNames } from './productNames'; +import { IInstallationChannelManager, IModuleInstaller } from './types'; + +@injectable() +export class InstallationChannelManager implements IInstallationChannelManager { + constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) { } + + public async getInstallationChannel(product: Product, resource?: Uri): Promise { + const channels = await this.getInstallationChannels(resource); + if (channels.length === 1) { + return channels[0]; + } + + const productName = ProductNames.get(product)!; + const appShell = this.serviceContainer.get(IApplicationShell); + if (channels.length === 0) { + appShell.showInformationMessage(`No installers available to install ${productName}.`); + return; + } + + const placeHolder = `Select an option to install ${productName}`; + const options = channels.map(installer => { + return { + label: `Install using ${installer.displayName}`, + description: '', + installer + } as QuickPickItem & { installer: IModuleInstaller }; + }); + const selection = await appShell.showQuickPick(options, { matchOnDescription: true, matchOnDetail: true, placeHolder }); + return selection ? selection.installer : undefined; + } + + public async getInstallationChannels(resource?: Uri): Promise { + const installers = this.serviceContainer.getAll(IModuleInstaller); + const supportedInstallers = await Promise.all(installers.map(async installer => installer.isSupported(resource).then(supported => supported ? installer : undefined))); + return supportedInstallers.filter(installer => installer !== undefined).map(installer => installer!); + } + + public async showNoInstallersMessage(resource?: Uri): Promise { + const interpreters = this.serviceContainer.get(IInterpreterService); + const interpreter = await interpreters.getActiveInterpreter(resource); + + const appShell = this.serviceContainer.get(IApplicationShell); + const search = 'Search for help'; + let result: string; + if (interpreter.type === InterpreterType.Conda) { + result = await appShell.showErrorMessage('There is no Conda or Pip installer available in the selected environment.', search); + } else { + result = await appShell.showErrorMessage('There is no Pip installer available in the selected environment.', search); + } + if (result === search) { + const platform = this.serviceContainer.get(IPlatformService); + const osName = platform.isWindows + ? 'Windows' + : (platform.isMac ? 'MacOS' : 'Linux'); + appShell.openUrl(`https://www.bing.com/search?q=Install Pip ${osName} ${(interpreter.type === InterpreterType.Conda) ? 'Conda' : ''}`); + } + } +} diff --git a/src/client/common/installer/condaInstaller.ts b/src/client/common/installer/condaInstaller.ts index 58ca0bddcdbf..b95ba40652c5 100644 --- a/src/client/common/installer/condaInstaller.ts +++ b/src/client/common/installer/condaInstaller.ts @@ -15,7 +15,7 @@ export class CondaInstaller extends ModuleInstaller implements IModuleInstaller public get displayName() { return 'Conda'; } - constructor( @inject(IServiceContainer) serviceContainer: IServiceContainer) { + constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) { super(serviceContainer); } /** @@ -27,16 +27,14 @@ export class CondaInstaller extends ModuleInstaller implements IModuleInstaller * @returns {Promise} Whether conda is supported as a module installer or not. */ public async isSupported(resource?: Uri): Promise { - if (typeof this.isCondaAvailable === 'boolean') { - return this.isCondaAvailable!; + if (!this.isCondaAvailable) { + return false; } const condaLocator = this.serviceContainer.get(ICondaService); const available = await condaLocator.isCondaAvailable(); - if (!available) { return false; } - // Now we need to check if the current environment is a conda environment or not. return this.isCurrentEnvironmentACondaEnvironment(resource); } diff --git a/src/client/common/installer/installer.ts b/src/client/common/installer/installer.ts index 0409af1fa737..473305f59bc3 100644 --- a/src/client/common/installer/installer.ts +++ b/src/client/common/installer/installer.ts @@ -13,7 +13,7 @@ import { IPlatformService } from '../platform/types'; import { IProcessService, IPythonExecutionFactory } from '../process/types'; import { ITerminalServiceFactory } from '../terminal/types'; import { IInstaller, ILogger, InstallerResponse, IOutputChannel, ModuleNamePurpose, Product } from '../types'; -import { IModuleInstaller } from './types'; +import { IInstallationChannelManager, IModuleInstaller } from './types'; export { Product } from '../types'; @@ -71,7 +71,7 @@ ProductTypes.set(Product.rope, ProductType.RefactoringLibrary); @injectable() export class Installer implements IInstaller { - constructor( @inject(IServiceContainer) private serviceContainer: IServiceContainer, + constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer, @inject(IOutputChannel) @named(STANDARD_OUTPUT_CHANNEL) private outputChannel: vscode.OutputChannel) { } // tslint:disable-next-line:no-empty @@ -135,7 +135,9 @@ export class Installer implements IInstaller { if (product === Product.ctags) { return this.installCTags(); } - const installer = await this.getInstallationChannel(product, resource); + + const channels = this.serviceContainer.get(IInstallationChannelManager); + const installer = await channels.getInstallationChannel(product, resource); if (!installer) { return InstallerResponse.Ignore; } @@ -191,32 +193,7 @@ export class Installer implements IInstaller { } return InstallerResponse.Ignore; } - private async getInstallationChannel(product: Product, resource?: Uri): Promise { - const productName = ProductNames.get(product)!; - const channels = await this.getInstallationChannels(resource); - if (channels.length === 0) { - window.showInformationMessage(`No installers available to install ${productName}.`); - return; - } - if (channels.length === 1) { - return channels[0]; - } - const placeHolder = `Select an option to install ${productName}`; - const options = channels.map(installer => { - return { - label: `Install using ${installer.displayName}`, - description: '', - installer - } as QuickPickItem & { installer: IModuleInstaller }; - }); - const selection = await window.showQuickPick(options, { matchOnDescription: true, matchOnDetail: true, placeHolder }); - return selection ? selection.installer : undefined; - } - private async getInstallationChannels(resource?: Uri): Promise { - const installers = this.serviceContainer.getAll(IModuleInstaller); - const supportedInstallers = await Promise.all(installers.map(async installer => installer.isSupported(resource).then(supported => supported ? installer : undefined))); - return supportedInstallers.filter(installer => installer !== undefined).map(installer => installer!); - } + // tslint:disable-next-line:no-any private updateSetting(setting: string, value: any, resource?: Uri) { if (resource && workspace.getWorkspaceFolder(resource)) { diff --git a/src/client/common/installer/productInstaller.ts b/src/client/common/installer/productInstaller.ts index 3991a882a4ff..d54c7a95ae31 100644 --- a/src/client/common/installer/productInstaller.ts +++ b/src/client/common/installer/productInstaller.ts @@ -13,27 +13,13 @@ import { IPlatformService } from '../platform/types'; import { IProcessService, IPythonExecutionFactory } from '../process/types'; import { ITerminalServiceFactory } from '../terminal/types'; import { IConfigurationService, IInstaller, ILogger, InstallerResponse, IOutputChannel, ModuleNamePurpose, Product } from '../types'; -import { IModuleInstaller } from './types'; +import { ProductNames } from './productNames'; +import { IInstallationChannelManager, IModuleInstaller } from './types'; export { Product } from '../types'; const CTagsInsllationScript = os.platform() === 'darwin' ? 'brew install ctags' : 'sudo apt-get install exuberant-ctags'; -// tslint:disable-next-line:variable-name -const ProductNames = new Map(); -ProductNames.set(Product.autopep8, 'autopep8'); -ProductNames.set(Product.flake8, 'flake8'); -ProductNames.set(Product.mypy, 'mypy'); -ProductNames.set(Product.nosetest, 'nosetest'); -ProductNames.set(Product.pep8, 'pep8'); -ProductNames.set(Product.pylama, 'pylama'); -ProductNames.set(Product.prospector, 'prospector'); -ProductNames.set(Product.pydocstyle, 'pydocstyle'); -ProductNames.set(Product.pylint, 'pylint'); -ProductNames.set(Product.pytest, 'pytest'); -ProductNames.set(Product.yapf, 'yapf'); -ProductNames.set(Product.rope, 'rope'); - enum ProductType { Linter, Formatter, @@ -59,7 +45,8 @@ abstract class BaseInstaller { return InstallerResponse.Installed; } - const installer = await this.getInstallationChannel(product, resource); + const channels = this.serviceContainer.get(IInstallationChannelManager); + const installer = await channels.getInstallationChannel(product, resource); if (!installer) { return InstallerResponse.Ignore; } @@ -100,34 +87,6 @@ abstract class BaseInstaller { protected getExecutableNameFromSettings(product: Product, resource?: Uri): string { throw new Error('getExecutableNameFromSettings is not supported on this object'); } - - private async getInstallationChannel(product: Product, resource?: Uri): Promise { - const productName = ProductNames.get(product)!; - const channels = await this.getInstallationChannels(resource); - if (channels.length === 0) { - window.showInformationMessage(`No installers available to install ${productName}.`); - return; - } - if (channels.length === 1) { - return channels[0]; - } - const placeHolder = `Select an option to install ${productName}`; - const options = channels.map(installer => { - return { - label: `Install using ${installer.displayName}`, - description: '', - installer - } as QuickPickItem & { installer: IModuleInstaller }; - }); - const selection = await window.showQuickPick(options, { matchOnDescription: true, matchOnDetail: true, placeHolder }); - return selection ? selection.installer : undefined; - } - - private async getInstallationChannels(resource?: Uri): Promise { - const installers = this.serviceContainer.getAll(IModuleInstaller); - const supportedInstallers = await Promise.all(installers.map(async installer => installer.isSupported(resource).then(supported => supported ? installer : undefined))); - return supportedInstallers.filter(installer => installer !== undefined).map(installer => installer!); - } } class CTagsInstaller extends BaseInstaller { @@ -253,7 +212,7 @@ class RefactoringLibraryInstaller extends BaseInstaller { export class ProductInstaller implements IInstaller { private ProductTypes = new Map(); - constructor( @inject(IServiceContainer) private serviceContainer: IServiceContainer, + constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer, @inject(IOutputChannel) @named(STANDARD_OUTPUT_CHANNEL) private outputChannel: vscode.OutputChannel) { this.ProductTypes.set(Product.flake8, ProductType.Linter); this.ProductTypes.set(Product.mypy, ProductType.Linter); diff --git a/src/client/common/installer/productNames.ts b/src/client/common/installer/productNames.ts new file mode 100644 index 000000000000..9371f540c778 --- /dev/null +++ b/src/client/common/installer/productNames.ts @@ -0,0 +1,19 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { Product } from '../types'; + +// tslint:disable-next-line:variable-name +export const ProductNames = new Map(); +ProductNames.set(Product.autopep8, 'autopep8'); +ProductNames.set(Product.flake8, 'flake8'); +ProductNames.set(Product.mypy, 'mypy'); +ProductNames.set(Product.nosetest, 'nosetest'); +ProductNames.set(Product.pep8, 'pep8'); +ProductNames.set(Product.pylama, 'pylama'); +ProductNames.set(Product.prospector, 'prospector'); +ProductNames.set(Product.pydocstyle, 'pydocstyle'); +ProductNames.set(Product.pylint, 'pylint'); +ProductNames.set(Product.pytest, 'pytest'); +ProductNames.set(Product.yapf, 'yapf'); +ProductNames.set(Product.rope, 'rope'); diff --git a/src/client/common/installer/pythonInstallation.ts b/src/client/common/installer/pythonInstallation.ts index bf3bd1384370..c177ae677e3d 100644 --- a/src/client/common/installer/pythonInstallation.ts +++ b/src/client/common/installer/pythonInstallation.ts @@ -3,6 +3,7 @@ 'use strict'; import { IInterpreterLocatorService, INTERPRETER_LOCATOR_SERVICE, InterpreterType } from '../../interpreter/contracts'; +import { isMacDefaultPythonPath } from '../../interpreter/locators/helpers'; import { IServiceContainer } from '../../ioc/types'; import { IApplicationShell } from '../application/types'; import { IPlatformService } from '../platform/types'; @@ -15,7 +16,7 @@ export class PythonInstaller { constructor(private serviceContainer: IServiceContainer) { this.locator = serviceContainer.get(IInterpreterLocatorService, INTERPRETER_LOCATOR_SERVICE); this.shell = serviceContainer.get(IApplicationShell); - } + } public async checkPythonInstallation(settings: IPythonSettings): Promise { if (settings.disableInstallationChecks === true) { @@ -25,7 +26,7 @@ export class PythonInstaller { if (interpreters.length > 0) { const platform = this.serviceContainer.get(IPlatformService); if (platform.isMac && - settings.pythonPath === 'python' && + isMacDefaultPythonPath(settings.pythonPath) && interpreters[0].type === InterpreterType.Unknown) { await this.shell.showWarningMessage('Selected interpreter is macOS system Python which is not recommended. Please select different interpreter'); } diff --git a/src/client/common/installer/serviceRegistry.ts b/src/client/common/installer/serviceRegistry.ts index c53d457a3829..0282d033fd0a 100644 --- a/src/client/common/installer/serviceRegistry.ts +++ b/src/client/common/installer/serviceRegistry.ts @@ -3,11 +3,13 @@ 'use strict'; import { IServiceManager } from '../../ioc/types'; +import { InstallationChannelManager } from './channelManager'; import { CondaInstaller } from './condaInstaller'; import { PipInstaller } from './pipInstaller'; -import { IModuleInstaller } from './types'; +import { IInstallationChannelManager, IModuleInstaller } from './types'; export function registerTypes(serviceManager: IServiceManager) { serviceManager.addSingleton(IModuleInstaller, CondaInstaller); serviceManager.addSingleton(IModuleInstaller, PipInstaller); + serviceManager.addSingleton(IInstallationChannelManager, InstallationChannelManager); } diff --git a/src/client/common/installer/types.ts b/src/client/common/installer/types.ts index a1759606de8b..7058dbe1c463 100644 --- a/src/client/common/installer/types.ts +++ b/src/client/common/installer/types.ts @@ -2,6 +2,7 @@ // Licensed under the MIT License. import { Uri } from 'vscode'; +import { Product } from '../types'; export const IModuleInstaller = Symbol('IModuleInstaller'); export interface IModuleInstaller { @@ -14,3 +15,10 @@ export const IPythonInstallation = Symbol('IPythonInstallation'); export interface IPythonInstallation { checkInstallation(): Promise; } + +export const IInstallationChannelManager = Symbol('IInstallationChannelManager'); +export interface IInstallationChannelManager { + getInstallationChannel(product: Product, resource?: Uri): Promise; + getInstallationChannels(resource?: Uri): Promise; + showNoInstallersMessage(): void; +} diff --git a/src/client/interpreter/locators/helpers.ts b/src/client/interpreter/locators/helpers.ts index 4efeb34d9721..a1d1dc7f6898 100644 --- a/src/client/interpreter/locators/helpers.ts +++ b/src/client/interpreter/locators/helpers.ts @@ -22,3 +22,7 @@ export function fixInterpreterDisplayName(item: PythonInterpreter) { } return item; } + +export function isMacDefaultPythonPath(p: string) { + return p === 'python' || p === '/usr/local/python'; +} diff --git a/src/client/interpreter/locators/services/baseVirtualEnvService.ts b/src/client/interpreter/locators/services/baseVirtualEnvService.ts index a060bf8502da..f4d2c2149598 100644 --- a/src/client/interpreter/locators/services/baseVirtualEnvService.ts +++ b/src/client/interpreter/locators/services/baseVirtualEnvService.ts @@ -54,10 +54,7 @@ export class BaseVirtualEnvService extends CacheableLocatorService { .then(dirs => { const scriptOrBinDirs = dirs.filter(dir => { const folderName = path.basename(dir); - // Perform case insistive search on windows. - // On windows its named eitgher 'Scripts' or 'scripts'. - const folderNameToCheck = isWindows ? folderName.toUpperCase() : folderName; - return folderNameToCheck === dirToLookFor; + return this.fileSystem.arePathsSame(folderName, dirToLookFor); }); return scriptOrBinDirs.length === 1 ? scriptOrBinDirs[0] : ''; }) @@ -68,18 +65,14 @@ export class BaseVirtualEnvService extends CacheableLocatorService { })); } private async getVirtualEnvDetails(interpreter: string): Promise { - return Promise.all([ - this.versionProvider.getVersion(interpreter, path.basename(interpreter)), - this.virtualEnvMgr.detect(interpreter) - ]) - .then(([displayName, virtualEnv]) => { - const virtualEnvSuffix = virtualEnv ? virtualEnv.name : this.getVirtualEnvironmentRootDirectory(interpreter); - return { - displayName: `${displayName} (${virtualEnvSuffix})`.trim(), - path: interpreter, - type: virtualEnv ? virtualEnv.type : InterpreterType.Unknown - }; - }); + const displayName = await this.versionProvider.getVersion(interpreter, path.basename(interpreter)); + const virtualEnv = await this.virtualEnvMgr.detect(interpreter); + const virtualEnvSuffix = virtualEnv ? virtualEnv.name : this.getVirtualEnvironmentRootDirectory(interpreter); + return { + displayName: `${displayName} (${virtualEnvSuffix})`.trim(), + path: interpreter, + type: virtualEnv ? virtualEnv.type : InterpreterType.Unknown + }; } private getVirtualEnvironmentRootDirectory(interpreter: string) { // Python interperters are always in a subdirectory of the environment folder. diff --git a/src/test/initialize.ts b/src/test/initialize.ts index 5c172a259f4e..863d3b021247 100644 --- a/src/test/initialize.ts +++ b/src/test/initialize.ts @@ -63,7 +63,7 @@ function getPythonPath(): string { // tslint:disable-next-line:no-unsafe-any return process.env.TRAVIS_PYTHON_PATH; } - return 'python'; + return '/usr/local/bin/python3'; } function isMultitrootTest() { diff --git a/src/test/install/channelManager.test.ts b/src/test/install/channelManager.test.ts new file mode 100644 index 000000000000..03795eadf319 --- /dev/null +++ b/src/test/install/channelManager.test.ts @@ -0,0 +1,127 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import * as assert from 'assert'; +import { Container } from 'inversify'; +import * as TypeMoq from 'typemoq'; +import { IApplicationShell } from '../../client/common/application/types'; +import { InstallationChannelManager } from '../../client/common/installer/channelManager'; +import { IPlatformService } from '../../client/common/platform/types'; +import { IInterpreterService, InterpreterType, PythonInterpreter } from '../../client/interpreter/contracts'; +import { ServiceContainer } from '../../client/ioc/container'; +import { ServiceManager } from '../../client/ioc/serviceManager'; +import { IServiceContainer } from '../../client/ioc/types'; + +// tslint:disable-next-line:max-func-body-length +suite('Installation - channels', () => { + let serviceContainer: IServiceContainer; + let platform: TypeMoq.IMock; + let appShell: TypeMoq.IMock; + let interpreters: TypeMoq.IMock; + + setup(() => { + const cont = new Container(); + const serviceManager = new ServiceManager(cont); + serviceContainer = new ServiceContainer(cont); + + platform = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(IPlatformService, platform.object); + + appShell = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(IApplicationShell, appShell.object); + + interpreters = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(IInterpreterService, interpreters.object); + }); + + test('No installers message: Unknown/Windows', async () => { + platform.setup(x => x.isWindows).returns(() => true); + await testInstallerMissingMessage(InterpreterType.Unknown, (message: string, url: string) => { + verifyMessage(message, ['Pip'], ['Conda']); + verifyUrl(url, ['Windows', 'Pip']); + }); + }); + + test('No installers message: Conda/Windows', async () => { + platform.setup(x => x.isWindows).returns(() => true); + await testInstallerMissingMessage(InterpreterType.Conda, (message: string, url: string) => { + verifyMessage(message, ['Pip', 'Conda'], []); + verifyUrl(url, ['Windows', 'Pip', 'Conda']); + }); + }); + + test('No installers message: Unknown/Mac', async () => { + platform.setup(x => x.isMac).returns(() => true); + await testInstallerMissingMessage(InterpreterType.Unknown, (message: string, url: string) => { + verifyMessage(message, ['Pip'], ['Conda']); + verifyUrl(url, ['Mac', 'Pip']); + }); + }); + + test('No installers message: Conda/Mac', async () => { + platform.setup(x => x.isMac).returns(() => true); + await testInstallerMissingMessage(InterpreterType.Conda, (message: string, url: string) => { + verifyMessage(message, ['Pip', 'Conda'], []); + verifyUrl(url, ['Mac', 'Pip', 'Conda']); + }); + }); + + test('No installers message: Unknown/Linux', async () => { + platform.setup(x => x.isLinux).returns(() => true); + await testInstallerMissingMessage(InterpreterType.Unknown, (message: string, url: string) => { + verifyMessage(message, ['Pip'], ['Conda']); + verifyUrl(url, ['Linux', 'Pip']); + }); + }); + + test('No installers message: Conda/Linux', async () => { + platform.setup(x => x.isLinux).returns(() => true); + await testInstallerMissingMessage(InterpreterType.Conda, (message: string, url: string) => { + verifyMessage(message, ['Pip', 'Conda'], []); + verifyUrl(url, ['Linux', 'Pip', 'Conda']); + }); + }); + + function verifyMessage(message: string, present: string[], missing: string[]) { + for (const p of present) { + assert.equal(message.indexOf(p) >= 0, true, `Message does not contain ${p}.`); + } + for (const m of missing) { + assert.equal(message.indexOf(m) < 0, true, `Message incorrectly contains ${m}.`); + } + } + + function verifyUrl(url: string, terms: string[]) { + assert.equal(url.indexOf('https://') > 0, true, 'Search Url must be https.'); + for (const term of terms) { + assert.equal(url.indexOf(term) >= 0, true, `Search Url does not contain ${term}.`); + } + } + + async function testInstallerMissingMessage( + interpreterType: InterpreterType, + verify: (a: string, b: string) => void): Promise { + + const activeInterpreter: PythonInterpreter = { + type: interpreterType, + path: '' + }; + interpreters.setup(x => x.getActiveInterpreter(TypeMoq.It.isAny())) + .returns(() => new Promise((resolve, reject) => resolve(activeInterpreter))); + const channels = new InstallationChannelManager(serviceContainer); + + let url: string; + let message: string; + appShell + .setup(x => x.showErrorMessage(TypeMoq.It.isAnyString())) + .callback((s: string) => { + message = s; + }) + .returns(() => new Promise(resolve, reject) => resolve('Search for help')); + appShell.setup(x => x.openUrl(TypeMoq.It.isAnyString())).callback((s: string) => { + url = s; + }); + await channels.showNoInstallersMessage(); + verify(message, url); + } +}); From d0a449fa4c9055cc166db336bffee30c830e3367 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Fri, 16 Feb 2018 16:50:16 -0800 Subject: [PATCH 06/32] Test --- src/test/install/channelManager.test.ts | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/test/install/channelManager.test.ts b/src/test/install/channelManager.test.ts index 03795eadf319..d90e219dfa84 100644 --- a/src/test/install/channelManager.test.ts +++ b/src/test/install/channelManager.test.ts @@ -110,14 +110,16 @@ suite('Installation - channels', () => { .returns(() => new Promise((resolve, reject) => resolve(activeInterpreter))); const channels = new InstallationChannelManager(serviceContainer); - let url: string; - let message: string; + let url: string = ''; + let message: string = ''; + let search: string = ''; appShell - .setup(x => x.showErrorMessage(TypeMoq.It.isAnyString())) - .callback((s: string) => { - message = s; + .setup(x => x.showErrorMessage(TypeMoq.It.isAnyString(), TypeMoq.It.isAnyString())) + .callback((m: string, s: string) => { + message = m; + search = s; }) - .returns(() => new Promise(resolve, reject) => resolve('Search for help')); + .returns(() => new Promise((resolve, reject) => resolve(search))); appShell.setup(x => x.openUrl(TypeMoq.It.isAnyString())).callback((s: string) => { url = s; }); From 55197c3456a1eae288af955230e3857e47fb78e2 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Fri, 16 Feb 2018 17:18:50 -0800 Subject: [PATCH 07/32] Tests --- src/test/install/channelManager.test.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/test/install/channelManager.test.ts b/src/test/install/channelManager.test.ts index d90e219dfa84..caf0a912561c 100644 --- a/src/test/install/channelManager.test.ts +++ b/src/test/install/channelManager.test.ts @@ -51,6 +51,7 @@ suite('Installation - channels', () => { }); test('No installers message: Unknown/Mac', async () => { + platform.setup(x => x.isWindows).returns(() => false); platform.setup(x => x.isMac).returns(() => true); await testInstallerMissingMessage(InterpreterType.Unknown, (message: string, url: string) => { verifyMessage(message, ['Pip'], ['Conda']); @@ -59,6 +60,7 @@ suite('Installation - channels', () => { }); test('No installers message: Conda/Mac', async () => { + platform.setup(x => x.isWindows).returns(() => false); platform.setup(x => x.isMac).returns(() => true); await testInstallerMissingMessage(InterpreterType.Conda, (message: string, url: string) => { verifyMessage(message, ['Pip', 'Conda'], []); @@ -67,6 +69,8 @@ suite('Installation - channels', () => { }); test('No installers message: Unknown/Linux', async () => { + platform.setup(x => x.isWindows).returns(() => false); + platform.setup(x => x.isMac).returns(() => false); platform.setup(x => x.isLinux).returns(() => true); await testInstallerMissingMessage(InterpreterType.Unknown, (message: string, url: string) => { verifyMessage(message, ['Pip'], ['Conda']); @@ -75,6 +79,8 @@ suite('Installation - channels', () => { }); test('No installers message: Conda/Linux', async () => { + platform.setup(x => x.isWindows).returns(() => false); + platform.setup(x => x.isMac).returns(() => false); platform.setup(x => x.isLinux).returns(() => true); await testInstallerMissingMessage(InterpreterType.Conda, (message: string, url: string) => { verifyMessage(message, ['Pip', 'Conda'], []); @@ -92,7 +98,7 @@ suite('Installation - channels', () => { } function verifyUrl(url: string, terms: string[]) { - assert.equal(url.indexOf('https://') > 0, true, 'Search Url must be https.'); + assert.equal(url.indexOf('https://') >= 0, true, 'Search Url must be https.'); for (const term of terms) { assert.equal(url.indexOf(term) >= 0, true, `Search Url does not contain ${term}.`); } From f6a01235960ab92d685219c6dd07d1859c3de4ab Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Fri, 16 Feb 2018 17:59:10 -0800 Subject: [PATCH 08/32] Tests --- .../install/channelManager.channels.test.ts | 52 +++++++++++++++++++ ...est.ts => channelManager.messages.test.ts} | 0 2 files changed, 52 insertions(+) create mode 100644 src/test/install/channelManager.channels.test.ts rename src/test/install/{channelManager.test.ts => channelManager.messages.test.ts} (100%) diff --git a/src/test/install/channelManager.channels.test.ts b/src/test/install/channelManager.channels.test.ts new file mode 100644 index 000000000000..4f4353b03a01 --- /dev/null +++ b/src/test/install/channelManager.channels.test.ts @@ -0,0 +1,52 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import * as assert from 'assert'; +import { Container } from 'inversify'; +import * as TypeMoq from 'typemoq'; +import { InstallationChannelManager } from '../../client/common/installer/channelManager'; +import { IModuleInstaller } from '../../client/common/installer/types'; +import { ServiceContainer } from '../../client/ioc/container'; +import { ServiceManager } from '../../client/ioc/serviceManager'; +import { IServiceContainer } from '../../client/ioc/types'; + +// tslint:disable-next-line:max-func-body-length +suite('Installation - channels', () => { + let serviceManager: ServiceManager; + let serviceContainer: IServiceContainer; + + setup(() => { + const cont = new Container(); + serviceManager = new ServiceManager(cont); + serviceContainer = new ServiceContainer(cont); + }); + + test('Single channel', async () => { + const installer = mockInstaller(true, ''); + const cm = new InstallationChannelManager(serviceContainer); + const channels = await cm.getInstallationChannels(); + assert.equal(channels.length, 1, 'Incorrect number of channels'); + assert.equal(channels[0], installer.object, 'Incorrect installer'); + }); + + test('Multiple channels', async () => { + const installer1 = mockInstaller(true, '1'); + mockInstaller(false, '2'); + const installer3 = mockInstaller(true, '3'); + + const cm = new InstallationChannelManager(serviceContainer); + const channels = await cm.getInstallationChannels(); + assert.equal(channels.length, 2, 'Incorrect number of channels'); + assert.equal(channels[0], installer1.object, 'Incorrect installer 1'); + assert.equal(channels[0], installer3.object, 'Incorrect installer 2'); + }); + + function mockInstaller(supported: boolean, name: string): TypeMoq.IMock { + const installer = TypeMoq.Mock.ofType(); + installer + .setup(x => x.isSupported(TypeMoq.It.isAny())) + .returns(() => new Promise((resolve) => resolve(true))); + serviceManager.addSingletonInstance(IModuleInstaller, installer.object, name); + return installer; + } +}); diff --git a/src/test/install/channelManager.test.ts b/src/test/install/channelManager.messages.test.ts similarity index 100% rename from src/test/install/channelManager.test.ts rename to src/test/install/channelManager.messages.test.ts From d07d3efbe9579e0487abe3904d711fa7412e5b68 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Fri, 16 Feb 2018 18:08:32 -0800 Subject: [PATCH 09/32] Mac python path --- src/client/interpreter/locators/index.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/client/interpreter/locators/index.ts b/src/client/interpreter/locators/index.ts index 3645e15cee14..c48040036777 100644 --- a/src/client/interpreter/locators/index.ts +++ b/src/client/interpreter/locators/index.ts @@ -18,7 +18,7 @@ import { WINDOWS_REGISTRY_SERVICE, WORKSPACE_VIRTUAL_ENV_SERVICE } from '../contracts'; -import { fixInterpreterDisplayName } from './helpers'; +import { fixInterpreterDisplayName, isMacDefaultPythonPath } from './helpers'; @injectable() export class PythonInterpreterLocatorService implements IInterpreterLocatorService { @@ -45,7 +45,7 @@ export class PythonInterpreterLocatorService implements IInterpreterLocatorServi .map(fixInterpreterDisplayName) .map(item => { item.path = path.normalize(item.path); return item; }) .reduce((accumulator, current) => { - if (this.platform.isMac && current.path === '/usr/bin/python') { + if (this.platform.isMac && isMacDefaultPythonPath(current.path)) { return accumulator; } const existingItem = accumulator.find(item => arePathsSame(item.path, current.path)); From 2b0cc92da7be34cd99aeb54e30679516718f6ccd Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Fri, 16 Feb 2018 18:19:47 -0800 Subject: [PATCH 10/32] Tests --- src/client/common/installer/channelManager.ts | 14 +++++++++++--- src/test/install/channelManager.channels.test.ts | 6 +++--- src/test/install/channelManager.messages.test.ts | 2 +- 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/client/common/installer/channelManager.ts b/src/client/common/installer/channelManager.ts index 252f467319e8..e16f0d2b5f87 100644 --- a/src/client/common/installer/channelManager.ts +++ b/src/client/common/installer/channelManager.ts @@ -42,17 +42,25 @@ export class InstallationChannelManager implements IInstallationChannelManager { public async getInstallationChannels(resource?: Uri): Promise { const installers = this.serviceContainer.getAll(IModuleInstaller); - const supportedInstallers = await Promise.all(installers.map(async installer => installer.isSupported(resource).then(supported => supported ? installer : undefined))); - return supportedInstallers.filter(installer => installer !== undefined).map(installer => installer!); + const supportedInstallers: IModuleInstaller[] = []; + for (const mi of installers) { + if (await mi.isSupported(resource)) { + supportedInstallers.push(mi); + } + } + return supportedInstallers; } public async showNoInstallersMessage(resource?: Uri): Promise { const interpreters = this.serviceContainer.get(IInterpreterService); const interpreter = await interpreters.getActiveInterpreter(resource); + if (!interpreter) { + return; // Handled in the Python installation check. + } const appShell = this.serviceContainer.get(IApplicationShell); const search = 'Search for help'; - let result: string; + let result: string | undefined; if (interpreter.type === InterpreterType.Conda) { result = await appShell.showErrorMessage('There is no Conda or Pip installer available in the selected environment.', search); } else { diff --git a/src/test/install/channelManager.channels.test.ts b/src/test/install/channelManager.channels.test.ts index 4f4353b03a01..7b54232d9114 100644 --- a/src/test/install/channelManager.channels.test.ts +++ b/src/test/install/channelManager.channels.test.ts @@ -11,7 +11,7 @@ import { ServiceManager } from '../../client/ioc/serviceManager'; import { IServiceContainer } from '../../client/ioc/types'; // tslint:disable-next-line:max-func-body-length -suite('Installation - channels', () => { +suite('Installation - installation channels', () => { let serviceManager: ServiceManager; let serviceContainer: IServiceContainer; @@ -38,14 +38,14 @@ suite('Installation - channels', () => { const channels = await cm.getInstallationChannels(); assert.equal(channels.length, 2, 'Incorrect number of channels'); assert.equal(channels[0], installer1.object, 'Incorrect installer 1'); - assert.equal(channels[0], installer3.object, 'Incorrect installer 2'); + assert.equal(channels[1], installer3.object, 'Incorrect installer 2'); }); function mockInstaller(supported: boolean, name: string): TypeMoq.IMock { const installer = TypeMoq.Mock.ofType(); installer .setup(x => x.isSupported(TypeMoq.It.isAny())) - .returns(() => new Promise((resolve) => resolve(true))); + .returns(() => new Promise((resolve) => resolve(supported))); serviceManager.addSingletonInstance(IModuleInstaller, installer.object, name); return installer; } diff --git a/src/test/install/channelManager.messages.test.ts b/src/test/install/channelManager.messages.test.ts index caf0a912561c..f6e8a730428a 100644 --- a/src/test/install/channelManager.messages.test.ts +++ b/src/test/install/channelManager.messages.test.ts @@ -13,7 +13,7 @@ import { ServiceManager } from '../../client/ioc/serviceManager'; import { IServiceContainer } from '../../client/ioc/types'; // tslint:disable-next-line:max-func-body-length -suite('Installation - channels', () => { +suite('Installation - channel messages', () => { let serviceContainer: IServiceContainer; let platform: TypeMoq.IMock; let appShell: TypeMoq.IMock; From 3867ec2ef565b21f314227571b37cac5a09e36bf Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Fri, 16 Feb 2018 19:16:16 -0800 Subject: [PATCH 11/32] Tests --- src/client/common/installer/channelManager.ts | 2 +- .../install/channelManager.channels.test.ts | 31 ++++++++ .../install/channelManager.messages.test.ts | 76 ++++++++++++------- 3 files changed, 81 insertions(+), 28 deletions(-) diff --git a/src/client/common/installer/channelManager.ts b/src/client/common/installer/channelManager.ts index e16f0d2b5f87..3ee00df21419 100644 --- a/src/client/common/installer/channelManager.ts +++ b/src/client/common/installer/channelManager.ts @@ -24,7 +24,7 @@ export class InstallationChannelManager implements IInstallationChannelManager { const productName = ProductNames.get(product)!; const appShell = this.serviceContainer.get(IApplicationShell); if (channels.length === 0) { - appShell.showInformationMessage(`No installers available to install ${productName}.`); + await this.showNoInstallersMessage(resource); return; } diff --git a/src/test/install/channelManager.channels.test.ts b/src/test/install/channelManager.channels.test.ts index 7b54232d9114..8c7b67e32103 100644 --- a/src/test/install/channelManager.channels.test.ts +++ b/src/test/install/channelManager.channels.test.ts @@ -4,8 +4,11 @@ import * as assert from 'assert'; import { Container } from 'inversify'; import * as TypeMoq from 'typemoq'; +import { QuickPickOptions } from 'vscode'; +import { IApplicationShell } from '../../client/common/application/types'; import { InstallationChannelManager } from '../../client/common/installer/channelManager'; import { IModuleInstaller } from '../../client/common/installer/types'; +import { Product } from '../../client/common/types'; import { ServiceContainer } from '../../client/ioc/container'; import { ServiceManager } from '../../client/ioc/serviceManager'; import { IServiceContainer } from '../../client/ioc/types'; @@ -41,6 +44,34 @@ suite('Installation - installation channels', () => { assert.equal(channels[1], installer3.object, 'Incorrect installer 2'); }); + test('Select installer', async () => { + const installer1 = mockInstaller(true, '1'); + const installer2 = mockInstaller(true, '2'); + + const appShell = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(IApplicationShell, appShell.object); + + // tslint:disable-next-line:no-any + let items: any[] | undefined; + appShell + .setup(x => x.showQuickPick(TypeMoq.It.isAny(), TypeMoq.It.isAny())) + .callback((i: string[], o: QuickPickOptions) => { + items = i; + }) + .returns(() => new Promise((resolve, reject) => resolve(''))); + + installer1.setup(x => x.displayName).returns(() => 'Name 1'); + installer2.setup(x => x.displayName).returns(() => 'Name 2'); + + const cm = new InstallationChannelManager(serviceContainer); + await cm.getInstallationChannel(Product.pylint); + + assert.notEqual(items, undefined, 'showQuickPick not called'); + assert.equal(items!.length, 2, 'Incorrect number of installer shown'); + assert.notEqual(items![0]!.label!.indexOf('Name 1'), -1, 'Incorrect first installer name'); + assert.notEqual(items![1]!.label!.indexOf('Name 2'), -1, 'Incorrect second installer name'); + }); + function mockInstaller(supported: boolean, name: string): TypeMoq.IMock { const installer = TypeMoq.Mock.ofType(); installer diff --git a/src/test/install/channelManager.messages.test.ts b/src/test/install/channelManager.messages.test.ts index f6e8a730428a..de0c7cfdc1e4 100644 --- a/src/test/install/channelManager.messages.test.ts +++ b/src/test/install/channelManager.messages.test.ts @@ -7,6 +7,7 @@ import * as TypeMoq from 'typemoq'; import { IApplicationShell } from '../../client/common/application/types'; import { InstallationChannelManager } from '../../client/common/installer/channelManager'; import { IPlatformService } from '../../client/common/platform/types'; +import { Product } from '../../client/common/types'; import { IInterpreterService, InterpreterType, PythonInterpreter } from '../../client/interpreter/contracts'; import { ServiceContainer } from '../../client/ioc/container'; import { ServiceManager } from '../../client/ioc/serviceManager'; @@ -36,56 +37,78 @@ suite('Installation - channel messages', () => { test('No installers message: Unknown/Windows', async () => { platform.setup(x => x.isWindows).returns(() => true); - await testInstallerMissingMessage(InterpreterType.Unknown, (message: string, url: string) => { - verifyMessage(message, ['Pip'], ['Conda']); - verifyUrl(url, ['Windows', 'Pip']); - }); + await testInstallerMissingMessage(InterpreterType.Unknown, + async (channels: InstallationChannelManager, message: string, url: string) => { + await channels.showNoInstallersMessage(); + verifyMessage(message, ['Pip'], ['Conda']); + verifyUrl(url, ['Windows', 'Pip']); + }); }); test('No installers message: Conda/Windows', async () => { platform.setup(x => x.isWindows).returns(() => true); - await testInstallerMissingMessage(InterpreterType.Conda, (message: string, url: string) => { - verifyMessage(message, ['Pip', 'Conda'], []); - verifyUrl(url, ['Windows', 'Pip', 'Conda']); - }); + await testInstallerMissingMessage(InterpreterType.Conda, + async (channels: InstallationChannelManager, message: string, url: string) => { + await channels.showNoInstallersMessage(); + verifyMessage(message, ['Pip', 'Conda'], []); + verifyUrl(url, ['Windows', 'Pip', 'Conda']); + }); }); test('No installers message: Unknown/Mac', async () => { platform.setup(x => x.isWindows).returns(() => false); platform.setup(x => x.isMac).returns(() => true); - await testInstallerMissingMessage(InterpreterType.Unknown, (message: string, url: string) => { - verifyMessage(message, ['Pip'], ['Conda']); - verifyUrl(url, ['Mac', 'Pip']); - }); + await testInstallerMissingMessage(InterpreterType.Unknown, + async (channels: InstallationChannelManager, message: string, url: string) => { + await channels.showNoInstallersMessage(); + verifyMessage(message, ['Pip'], ['Conda']); + verifyUrl(url, ['Mac', 'Pip']); + }); }); test('No installers message: Conda/Mac', async () => { platform.setup(x => x.isWindows).returns(() => false); platform.setup(x => x.isMac).returns(() => true); - await testInstallerMissingMessage(InterpreterType.Conda, (message: string, url: string) => { - verifyMessage(message, ['Pip', 'Conda'], []); - verifyUrl(url, ['Mac', 'Pip', 'Conda']); - }); + await testInstallerMissingMessage(InterpreterType.Conda, + async (channels: InstallationChannelManager, message: string, url: string) => { + await channels.showNoInstallersMessage(); + verifyMessage(message, ['Pip', 'Conda'], []); + verifyUrl(url, ['Mac', 'Pip', 'Conda']); + }); }); test('No installers message: Unknown/Linux', async () => { platform.setup(x => x.isWindows).returns(() => false); platform.setup(x => x.isMac).returns(() => false); platform.setup(x => x.isLinux).returns(() => true); - await testInstallerMissingMessage(InterpreterType.Unknown, (message: string, url: string) => { - verifyMessage(message, ['Pip'], ['Conda']); - verifyUrl(url, ['Linux', 'Pip']); - }); + await testInstallerMissingMessage(InterpreterType.Unknown, + async (channels: InstallationChannelManager, message: string, url: string) => { + await channels.showNoInstallersMessage(); + verifyMessage(message, ['Pip'], ['Conda']); + verifyUrl(url, ['Linux', 'Pip']); + }); }); test('No installers message: Conda/Linux', async () => { platform.setup(x => x.isWindows).returns(() => false); platform.setup(x => x.isMac).returns(() => false); platform.setup(x => x.isLinux).returns(() => true); - await testInstallerMissingMessage(InterpreterType.Conda, (message: string, url: string) => { - verifyMessage(message, ['Pip', 'Conda'], []); - verifyUrl(url, ['Linux', 'Pip', 'Conda']); - }); + await testInstallerMissingMessage(InterpreterType.Conda, + async (channels: InstallationChannelManager, message: string, url: string) => { + await channels.showNoInstallersMessage(); + verifyMessage(message, ['Pip', 'Conda'], []); + verifyUrl(url, ['Linux', 'Pip', 'Conda']); + }); + }); + + test('No channels message', async () => { + platform.setup(x => x.isWindows).returns(() => true); + await testInstallerMissingMessage(InterpreterType.Unknown, + async (channels: InstallationChannelManager, message: string, url: string) => { + await channels.getInstallationChannel(Product.pylint); + verifyMessage(message, ['Pip'], ['Conda']); + verifyUrl(url, ['Windows', 'Pip']); + }); }); function verifyMessage(message: string, present: string[], missing: string[]) { @@ -106,7 +129,7 @@ suite('Installation - channel messages', () => { async function testInstallerMissingMessage( interpreterType: InterpreterType, - verify: (a: string, b: string) => void): Promise { + verify: (c: InstallationChannelManager, m: string, u: string) => void): Promise { const activeInterpreter: PythonInterpreter = { type: interpreterType, @@ -129,7 +152,6 @@ suite('Installation - channel messages', () => { appShell.setup(x => x.openUrl(TypeMoq.It.isAnyString())).callback((s: string) => { url = s; }); - await channels.showNoInstallersMessage(); - verify(message, url); + verify(channels, message, url); } }); From 85fc4ef985d21af3c9a7995effd8b0ce6f1eb13f Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Fri, 16 Feb 2018 19:17:04 -0800 Subject: [PATCH 12/32] Test --- src/test/install/channelManager.channels.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/install/channelManager.channels.test.ts b/src/test/install/channelManager.channels.test.ts index 8c7b67e32103..4c238aa20b43 100644 --- a/src/test/install/channelManager.channels.test.ts +++ b/src/test/install/channelManager.channels.test.ts @@ -58,7 +58,7 @@ suite('Installation - installation channels', () => { .callback((i: string[], o: QuickPickOptions) => { items = i; }) - .returns(() => new Promise((resolve, reject) => resolve(''))); + .returns(() => new Promise((resolve, reject) => resolve(undefined))); installer1.setup(x => x.displayName).returns(() => 'Name 1'); installer2.setup(x => x.displayName).returns(() => 'Name 2'); From 00887a47324aa587b081092b49cc4691bef614ff Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Fri, 16 Feb 2018 19:50:17 -0800 Subject: [PATCH 13/32] "Go To Python object" doesn't work --- src/client/languageServices/jediProxyFactory.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/languageServices/jediProxyFactory.ts b/src/client/languageServices/jediProxyFactory.ts index 8ac6b04349d5..569e57e66a75 100644 --- a/src/client/languageServices/jediProxyFactory.ts +++ b/src/client/languageServices/jediProxyFactory.ts @@ -16,7 +16,7 @@ export class JediFactory implements Disposable { this.disposables = []; } public getJediProxyHandler(resource: Uri): JediProxyHandler { - const workspaceFolder = workspace.getWorkspaceFolder(resource); + const workspaceFolder = resource ? workspace.getWorkspaceFolder(resource) : undefined; let workspacePath = workspaceFolder ? workspaceFolder.uri.fsPath : undefined; if (!workspacePath) { if (Array.isArray(workspace.workspaceFolders) && workspace.workspaceFolders.length > 0) { From f89dd96ebf9601a0b65e360604b3f349f2ea0a47 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Fri, 16 Feb 2018 20:35:11 -0800 Subject: [PATCH 14/32] Proper detection and type population in virtual env --- .../common/installer/pythonInstallation.ts | 12 ++++++---- src/client/interpreter/index.ts | 5 ++-- src/client/interpreter/locators/helpers.ts | 2 +- .../locators/services/currentPathService.ts | 22 ++++++++---------- src/client/interpreter/virtualEnvs/index.ts | 4 ++-- src/client/interpreter/virtualEnvs/types.ts | 2 +- src/client/interpreter/virtualEnvs/venv.ts | 15 +++++++++--- .../interpreter/virtualEnvs/virtualEnv.ts | 23 +++++++++++++------ 8 files changed, 51 insertions(+), 34 deletions(-) diff --git a/src/client/common/installer/pythonInstallation.ts b/src/client/common/installer/pythonInstallation.ts index c177ae677e3d..1463404eb0ab 100644 --- a/src/client/common/installer/pythonInstallation.ts +++ b/src/client/common/installer/pythonInstallation.ts @@ -2,7 +2,7 @@ // Licensed under the MIT License. 'use strict'; -import { IInterpreterLocatorService, INTERPRETER_LOCATOR_SERVICE, InterpreterType } from '../../interpreter/contracts'; +import { IInterpreterLocatorService, IInterpreterService, INTERPRETER_LOCATOR_SERVICE, InterpreterType } from '../../interpreter/contracts'; import { isMacDefaultPythonPath } from '../../interpreter/locators/helpers'; import { IServiceContainer } from '../../ioc/types'; import { IApplicationShell } from '../application/types'; @@ -25,10 +25,12 @@ export class PythonInstaller { const interpreters = await this.locator.getInterpreters(); if (interpreters.length > 0) { const platform = this.serviceContainer.get(IPlatformService); - if (platform.isMac && - isMacDefaultPythonPath(settings.pythonPath) && - interpreters[0].type === InterpreterType.Unknown) { - await this.shell.showWarningMessage('Selected interpreter is macOS system Python which is not recommended. Please select different interpreter'); + if (platform.isMac && isMacDefaultPythonPath(settings.pythonPath)) { + const interpreterService = this.serviceContainer.get(IInterpreterService); + const interpreter = await interpreterService.getActiveInterpreter(); + if (interpreter && interpreter.type === InterpreterType.Unknown) { + await this.shell.showWarningMessage('Selected interpreter is macOS system Python which is not recommended. Please select different interpreter'); + } } return true; } diff --git a/src/client/interpreter/index.ts b/src/client/interpreter/index.ts index 3eddee641c28..3c23ef5d807e 100644 --- a/src/client/interpreter/index.ts +++ b/src/client/interpreter/index.ts @@ -102,13 +102,14 @@ export class InterpreterManager implements Disposable, IInterpreterService { const pythonExecutableName = path.basename(fullyQualifiedPath); const versionInfo = await this.serviceContainer.get(IInterpreterVersionService).getVersion(fullyQualifiedPath, pythonExecutableName); const virtualEnvManager = this.serviceContainer.get(IVirtualEnvironmentManager); - const virtualEnvName = await virtualEnvManager.detect(fullyQualifiedPath).then(env => env ? env.name : ''); + const virtualEnv = await virtualEnvManager.detect(fullyQualifiedPath); + const virtualEnvName = virtualEnv ? virtualEnv.name : ''; const dislayNameSuffix = virtualEnvName.length > 0 ? ` (${virtualEnvName})` : ''; const displayName = `${versionInfo}${dislayNameSuffix}`; return { displayName, path: fullyQualifiedPath, - type: InterpreterType.Unknown, + type: virtualEnv ? virtualEnv.type : InterpreterType.Unknown, version: versionInfo }; } diff --git a/src/client/interpreter/locators/helpers.ts b/src/client/interpreter/locators/helpers.ts index a1d1dc7f6898..1b5cf698fcbe 100644 --- a/src/client/interpreter/locators/helpers.ts +++ b/src/client/interpreter/locators/helpers.ts @@ -24,5 +24,5 @@ export function fixInterpreterDisplayName(item: PythonInterpreter) { } export function isMacDefaultPythonPath(p: string) { - return p === 'python' || p === '/usr/local/python'; + return p === 'python' || p === '/usr/bin/python'; } diff --git a/src/client/interpreter/locators/services/currentPathService.ts b/src/client/interpreter/locators/services/currentPathService.ts index 62d934ff5634..e6d349c86600 100644 --- a/src/client/interpreter/locators/services/currentPathService.ts +++ b/src/client/interpreter/locators/services/currentPathService.ts @@ -11,7 +11,7 @@ import { CacheableLocatorService } from './cacheableLocatorService'; @injectable() export class CurrentPathService extends CacheableLocatorService { - public constructor( @inject(IVirtualEnvironmentManager) private virtualEnvMgr: IVirtualEnvironmentManager, + public constructor(@inject(IVirtualEnvironmentManager) private virtualEnvMgr: IVirtualEnvironmentManager, @inject(IInterpreterVersionService) private versionProvider: IInterpreterVersionService, @inject(IProcessService) private processService: IProcessService, @inject(IServiceContainer) serviceContainer: IServiceContainer) { @@ -35,18 +35,14 @@ export class CurrentPathService extends CacheableLocatorService { .then(interpreters => Promise.all(interpreters.map(interpreter => this.getInterpreterDetails(interpreter)))); } private async getInterpreterDetails(interpreter: string) { - return Promise.all([ - this.versionProvider.getVersion(interpreter, path.basename(interpreter)), - this.virtualEnvMgr.detect(interpreter) - ]) - .then(([displayName, virtualEnv]) => { - displayName += virtualEnv ? ` (${virtualEnv.name})` : ''; - return { - displayName, - path: interpreter, - type: InterpreterType.Unknown - }; - }); + let displayName = await this.versionProvider.getVersion(interpreter, path.basename(interpreter)); + const virtualEnv = await this.virtualEnvMgr.detect(interpreter); + displayName += virtualEnv ? ` (${virtualEnv.name})` : ''; + return { + displayName, + path: interpreter, + type: virtualEnv ? virtualEnv.type : InterpreterType.Unknown + }; } private async getInterpreter(pythonPath: string, defaultValue: string) { return this.processService.exec(pythonPath, ['-c', 'import sys;print(sys.executable)'], {}) diff --git a/src/client/interpreter/virtualEnvs/index.ts b/src/client/interpreter/virtualEnvs/index.ts index d0a9bd1634f6..01a538fbb012 100644 --- a/src/client/interpreter/virtualEnvs/index.ts +++ b/src/client/interpreter/virtualEnvs/index.ts @@ -3,9 +3,9 @@ import { IVirtualEnvironmentIdentifier, IVirtualEnvironmentManager } from './typ @injectable() export class VirtualEnvironmentManager implements IVirtualEnvironmentManager { - constructor( @multiInject(IVirtualEnvironmentIdentifier) private envs: IVirtualEnvironmentIdentifier[]) { + constructor(@multiInject(IVirtualEnvironmentIdentifier) private envs: IVirtualEnvironmentIdentifier[]) { } - public detect(pythonPath: string): Promise { + public detect(pythonPath: string): Promise { const promises = this.envs .map(item => item.detect(pythonPath) .then(result => { diff --git a/src/client/interpreter/virtualEnvs/types.ts b/src/client/interpreter/virtualEnvs/types.ts index 710507358999..10c44f9e8e96 100644 --- a/src/client/interpreter/virtualEnvs/types.ts +++ b/src/client/interpreter/virtualEnvs/types.ts @@ -11,5 +11,5 @@ export interface IVirtualEnvironmentIdentifier { } export const IVirtualEnvironmentManager = Symbol('VirtualEnvironmentManager'); export interface IVirtualEnvironmentManager { - detect(pythonPath: string): Promise; + detect(pythonPath: string): Promise; } diff --git a/src/client/interpreter/virtualEnvs/venv.ts b/src/client/interpreter/virtualEnvs/venv.ts index 934014c2d6c4..3fca7bb4f2c0 100644 --- a/src/client/interpreter/virtualEnvs/venv.ts +++ b/src/client/interpreter/virtualEnvs/venv.ts @@ -1,6 +1,10 @@ -import { injectable } from 'inversify'; +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { inject, injectable } from 'inversify'; import * as path from 'path'; -import { fsExistsAsync } from '../../common/utils'; +import { IFileSystem } from '../../common/platform/types'; +import { IServiceContainer } from '../../ioc/types'; import { InterpreterType } from '../contracts'; import { IVirtualEnvironmentIdentifier } from './types'; @@ -10,9 +14,14 @@ const pyEnvCfgFileName = 'pyvenv.cfg'; export class VEnv implements IVirtualEnvironmentIdentifier { public readonly name: string = 'venv'; public readonly type = InterpreterType.VEnv; + private fs: IFileSystem; + + constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) { + this.fs = serviceContainer.get(IFileSystem); + } public detect(pythonPath: string): Promise { const dir = path.dirname(pythonPath); const pyEnvCfgPath = path.join(dir, '..', pyEnvCfgFileName); - return fsExistsAsync(pyEnvCfgPath); + return this.fs.fileExistsAsync(pyEnvCfgPath); } } diff --git a/src/client/interpreter/virtualEnvs/virtualEnv.ts b/src/client/interpreter/virtualEnvs/virtualEnv.ts index e2c78f9e2e6e..e1dada31a344 100644 --- a/src/client/interpreter/virtualEnvs/virtualEnv.ts +++ b/src/client/interpreter/virtualEnvs/virtualEnv.ts @@ -1,18 +1,27 @@ -import { injectable } from 'inversify'; +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { inject, injectable } from 'inversify'; import * as path from 'path'; -import { fsExistsAsync } from '../../common/utils'; +import { IFileSystem } from '../../common/platform/types'; +import { IServiceContainer } from '../../ioc/types'; import { InterpreterType } from '../contracts'; import { IVirtualEnvironmentIdentifier } from './types'; -const OrigPrefixFile = 'orig-prefix.txt'; - @injectable() export class VirtualEnv implements IVirtualEnvironmentIdentifier { public readonly name: string = 'virtualenv'; public readonly type = InterpreterType.VirtualEnv; - public detect(pythonPath: string): Promise { + private fs: IFileSystem; + + constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) { + this.fs = serviceContainer.get(IFileSystem); + } + public async detect(pythonPath: string): Promise { const dir = path.dirname(pythonPath); - const origPrefixFile = path.join(dir, '..', 'lib', OrigPrefixFile); - return fsExistsAsync(origPrefixFile); + const libExists = await this.fs.directoryExistsAsync(path.join(dir, '..', 'lib')); + const binExists = await this.fs.directoryExistsAsync(path.join(dir, '..', 'bin')); + const includeExists = await this.fs.directoryExistsAsync(path.join(dir, '..', 'include')); + return libExists && binExists && includeExists; } } From 32394e273c860a7cced87858cb874d6890e4812c Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Sat, 17 Feb 2018 10:04:58 -0800 Subject: [PATCH 15/32] Test fixes --- src/client/common/installer/condaInstaller.ts | 14 +++++++------- src/test/common/installer.test.ts | 6 ++++-- src/test/install/channelManager.messages.test.ts | 3 ++- src/test/install/pythonInstallation.test.ts | 12 +++++++++++- 4 files changed, 24 insertions(+), 11 deletions(-) diff --git a/src/client/common/installer/condaInstaller.ts b/src/client/common/installer/condaInstaller.ts index b95ba40652c5..0c279d169510 100644 --- a/src/client/common/installer/condaInstaller.ts +++ b/src/client/common/installer/condaInstaller.ts @@ -3,7 +3,7 @@ import { inject, injectable } from 'inversify'; import { Uri } from 'vscode'; -import { ICondaService, IInterpreterService, InterpreterType } from '../../interpreter/contracts'; +import { ICondaService } from '../../interpreter/contracts'; import { IServiceContainer } from '../../ioc/types'; import { ExecutionInfo, IConfigurationService } from '../types'; import { ModuleInstaller } from './moduleInstaller'; @@ -27,12 +27,12 @@ export class CondaInstaller extends ModuleInstaller implements IModuleInstaller * @returns {Promise} Whether conda is supported as a module installer or not. */ public async isSupported(resource?: Uri): Promise { - if (!this.isCondaAvailable) { - return false; + if (this.isCondaAvailable !== undefined) { + return this.isCondaAvailable!; } const condaLocator = this.serviceContainer.get(ICondaService); - const available = await condaLocator.isCondaAvailable(); - if (!available) { + this.isCondaAvailable = await condaLocator.isCondaAvailable(); + if (!this.isCondaAvailable) { return false; } // Now we need to check if the current environment is a conda environment or not. @@ -46,11 +46,11 @@ export class CondaInstaller extends ModuleInstaller implements IModuleInstaller const info = await condaService.getCondaEnvironment(pythonPath); const args = ['install']; - if (info.name) { + if (info && info.name) { // If we have the name of the conda environment, then use that. args.push('--name'); args.push(info.name!); - } else if (info.path) { + } else if (info && info.path) { // Else provide the full path to the environment path. args.push('--prefix'); args.push(info.path); diff --git a/src/test/common/installer.test.ts b/src/test/common/installer.test.ts index 4af691063219..d00258561d34 100644 --- a/src/test/common/installer.test.ts +++ b/src/test/common/installer.test.ts @@ -5,16 +5,17 @@ import { IApplicationShell } from '../../client/common/application/types'; import { ConfigurationService } from '../../client/common/configuration/service'; import { EnumEx } from '../../client/common/enumUtils'; import { createDeferred } from '../../client/common/helpers'; +import { InstallationChannelManager } from '../../client/common/installer/channelManager'; import { ProductInstaller } from '../../client/common/installer/productInstaller'; -import { IModuleInstaller } from '../../client/common/installer/types'; +import { IInstallationChannelManager, IModuleInstaller } from '../../client/common/installer/types'; import { Logger } from '../../client/common/logger'; import { PersistentStateFactory } from '../../client/common/persistentState'; import { PathUtils } from '../../client/common/platform/pathUtils'; import { CurrentProcess } from '../../client/common/process/currentProcess'; import { IProcessService } from '../../client/common/process/types'; import { IConfigurationService, ICurrentProcess, IInstaller, ILogger, IPathUtils, IPersistentStateFactory, IsWindows, ModuleNamePurpose, Product } from '../../client/common/types'; -import { updateSetting } from '../common'; import { rootWorkspaceUri } from '../common'; +import { updateSetting } from '../common'; import { MockModuleInstaller } from '../mocks/moduleInstaller'; import { MockProcessService } from '../mocks/proc'; import { UnitTestIocContainer } from '../unittests/serviceRegistry'; @@ -53,6 +54,7 @@ suite('Installer', () => { ioc.serviceManager.addSingleton(IInstaller, ProductInstaller); ioc.serviceManager.addSingleton(IPathUtils, PathUtils); ioc.serviceManager.addSingleton(ICurrentProcess, CurrentProcess); + ioc.serviceManager.addSingleton(IInstallationChannelManager, InstallationChannelManager); ioc.serviceManager.addSingletonInstance(IApplicationShell, TypeMoq.Mock.ofType().object); ioc.serviceManager.addSingleton(IConfigurationService, ConfigurationService); diff --git a/src/test/install/channelManager.messages.test.ts b/src/test/install/channelManager.messages.test.ts index de0c7cfdc1e4..0762b4242014 100644 --- a/src/test/install/channelManager.messages.test.ts +++ b/src/test/install/channelManager.messages.test.ts @@ -135,7 +135,8 @@ suite('Installation - channel messages', () => { type: interpreterType, path: '' }; - interpreters.setup(x => x.getActiveInterpreter(TypeMoq.It.isAny())) + interpreters + .setup(x => x.getActiveInterpreter(TypeMoq.It.isAny())) .returns(() => new Promise((resolve, reject) => resolve(activeInterpreter))); const channels = new InstallationChannelManager(serviceContainer); diff --git a/src/test/install/pythonInstallation.test.ts b/src/test/install/pythonInstallation.test.ts index f3114b961f8c..4df7dfdd4fb2 100644 --- a/src/test/install/pythonInstallation.test.ts +++ b/src/test/install/pythonInstallation.test.ts @@ -9,7 +9,7 @@ import { IApplicationShell } from '../../client/common/application/types'; import { PythonInstaller } from '../../client/common/installer/pythonInstallation'; import { IPlatformService } from '../../client/common/platform/types'; import { IPythonSettings } from '../../client/common/types'; -import { IInterpreterLocatorService } from '../../client/interpreter/contracts'; +import { IInterpreterLocatorService, IInterpreterService } from '../../client/interpreter/contracts'; import { InterpreterType, PythonInterpreter } from '../../client/interpreter/contracts'; import { ServiceContainer } from '../../client/ioc/container'; import { ServiceManager } from '../../client/ioc/serviceManager'; @@ -35,9 +35,19 @@ class TestContext { this.locator = TypeMoq.Mock.ofType(); this.settings = TypeMoq.Mock.ofType(); + const activeInterpreter: PythonInterpreter = { + type: InterpreterType.Unknown, + path: '' + }; + const interpreterService = TypeMoq.Mock.ofType(); + interpreterService + .setup(x => x.getActiveInterpreter(TypeMoq.It.isAny())) + .returns(() => new Promise((resolve, reject) => resolve(activeInterpreter))); + this.serviceManager.addSingletonInstance(IPlatformService, this.platform.object); this.serviceManager.addSingletonInstance(IApplicationShell, this.appShell.object); this.serviceManager.addSingletonInstance(IInterpreterLocatorService, this.locator.object); + this.serviceManager.addSingletonInstance(IInterpreterService, interpreterService.object); this.pythonInstaller = new PythonInstaller(this.serviceContainer); this.platform.setup(x => x.isMac).returns(() => isMac); From 2e9c039d2339463b5bdfd1727ab56176eff7d3c9 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Sat, 17 Feb 2018 11:44:12 -0800 Subject: [PATCH 16/32] Simplify venv check --- src/client/interpreter/contracts.ts | 3 +- src/client/interpreter/display/index.ts | 6 ++-- src/client/interpreter/index.ts | 5 ++- .../services/baseVirtualEnvService.ts | 6 ++-- .../locators/services/currentPathService.ts | 6 ++-- src/client/interpreter/serviceRegistry.ts | 7 +--- src/client/interpreter/virtualEnvs/index.ts | 35 +++++++++++-------- src/client/interpreter/virtualEnvs/types.ts | 10 +----- src/client/interpreter/virtualEnvs/venv.ts | 27 -------------- .../interpreter/virtualEnvs/virtualEnv.ts | 27 -------------- src/test/interpreters/display.test.ts | 6 ++-- src/test/interpreters/mocks.ts | 12 +------ 12 files changed, 37 insertions(+), 113 deletions(-) delete mode 100644 src/client/interpreter/virtualEnvs/venv.ts delete mode 100644 src/client/interpreter/virtualEnvs/virtualEnv.ts diff --git a/src/client/interpreter/contracts.ts b/src/client/interpreter/contracts.ts index 727415e00b0e..fc3138bf1170 100644 --- a/src/client/interpreter/contracts.ts +++ b/src/client/interpreter/contracts.ts @@ -53,8 +53,7 @@ export interface ICondaService { export enum InterpreterType { Unknown = 1, Conda = 2, - VirtualEnv = 4, - VEnv = 8 + VirtualEnv = 4 } export type PythonInterpreter = { diff --git a/src/client/interpreter/display/index.ts b/src/client/interpreter/display/index.ts index 6ddaf6edc13c..2c73c46bb0ae 100644 --- a/src/client/interpreter/display/index.ts +++ b/src/client/interpreter/display/index.ts @@ -81,9 +81,7 @@ export class InterpreterDisplay implements IInterpreterDisplay { } this.statusBar.show(); } - private async getVirtualEnvironmentName(pythonPath: string) { - return this.virtualEnvMgr - .detect(pythonPath) - .then(env => env ? env.name : ''); + private async getVirtualEnvironmentName(pythonPath: string): Promise { + return this.virtualEnvMgr.detect(pythonPath); } } diff --git a/src/client/interpreter/index.ts b/src/client/interpreter/index.ts index 3c23ef5d807e..9d50eeb5620e 100644 --- a/src/client/interpreter/index.ts +++ b/src/client/interpreter/index.ts @@ -102,14 +102,13 @@ export class InterpreterManager implements Disposable, IInterpreterService { const pythonExecutableName = path.basename(fullyQualifiedPath); const versionInfo = await this.serviceContainer.get(IInterpreterVersionService).getVersion(fullyQualifiedPath, pythonExecutableName); const virtualEnvManager = this.serviceContainer.get(IVirtualEnvironmentManager); - const virtualEnv = await virtualEnvManager.detect(fullyQualifiedPath); - const virtualEnvName = virtualEnv ? virtualEnv.name : ''; + const virtualEnvName = await virtualEnvManager.detect(fullyQualifiedPath); const dislayNameSuffix = virtualEnvName.length > 0 ? ` (${virtualEnvName})` : ''; const displayName = `${versionInfo}${dislayNameSuffix}`; return { displayName, path: fullyQualifiedPath, - type: virtualEnv ? virtualEnv.type : InterpreterType.Unknown, + type: virtualEnvName.length > 0 ? InterpreterType.VirtualEnv : InterpreterType.Unknown, version: versionInfo }; } diff --git a/src/client/interpreter/locators/services/baseVirtualEnvService.ts b/src/client/interpreter/locators/services/baseVirtualEnvService.ts index f4d2c2149598..3a95fe74351e 100644 --- a/src/client/interpreter/locators/services/baseVirtualEnvService.ts +++ b/src/client/interpreter/locators/services/baseVirtualEnvService.ts @@ -66,12 +66,12 @@ export class BaseVirtualEnvService extends CacheableLocatorService { } private async getVirtualEnvDetails(interpreter: string): Promise { const displayName = await this.versionProvider.getVersion(interpreter, path.basename(interpreter)); - const virtualEnv = await this.virtualEnvMgr.detect(interpreter); - const virtualEnvSuffix = virtualEnv ? virtualEnv.name : this.getVirtualEnvironmentRootDirectory(interpreter); + const virtualEnvName = await this.virtualEnvMgr.detect(interpreter); + const virtualEnvSuffix = virtualEnvName.length > 0 ? virtualEnvName : this.getVirtualEnvironmentRootDirectory(interpreter); return { displayName: `${displayName} (${virtualEnvSuffix})`.trim(), path: interpreter, - type: virtualEnv ? virtualEnv.type : InterpreterType.Unknown + type: virtualEnvName.length > 0 ? InterpreterType.VirtualEnv : InterpreterType.Unknown }; } private getVirtualEnvironmentRootDirectory(interpreter: string) { diff --git a/src/client/interpreter/locators/services/currentPathService.ts b/src/client/interpreter/locators/services/currentPathService.ts index e6d349c86600..16233ac43a16 100644 --- a/src/client/interpreter/locators/services/currentPathService.ts +++ b/src/client/interpreter/locators/services/currentPathService.ts @@ -36,12 +36,12 @@ export class CurrentPathService extends CacheableLocatorService { } private async getInterpreterDetails(interpreter: string) { let displayName = await this.versionProvider.getVersion(interpreter, path.basename(interpreter)); - const virtualEnv = await this.virtualEnvMgr.detect(interpreter); - displayName += virtualEnv ? ` (${virtualEnv.name})` : ''; + const virtualEnvName = await this.virtualEnvMgr.detect(interpreter); + displayName += virtualEnvName.length > 0 ? ` (${virtualEnvName})` : ''; return { displayName, path: interpreter, - type: virtualEnv ? virtualEnv.type : InterpreterType.Unknown + type: virtualEnvName ? InterpreterType.VirtualEnv : InterpreterType.Unknown }; } private async getInterpreter(pythonPath: string, defaultValue: string) { diff --git a/src/client/interpreter/serviceRegistry.ts b/src/client/interpreter/serviceRegistry.ts index 7c4c0cbb55c6..56e60c0b6b35 100644 --- a/src/client/interpreter/serviceRegistry.ts +++ b/src/client/interpreter/serviceRegistry.ts @@ -41,9 +41,7 @@ import { getKnownSearchPathsForInterpreters, KnownPathsService } from './locator import { WindowsRegistryService } from './locators/services/windowsRegistryService'; import { WorkspaceVirtualEnvironmentsSearchPathProvider, WorkspaceVirtualEnvService } from './locators/services/workspaceVirtualEnvService'; import { VirtualEnvironmentManager } from './virtualEnvs/index'; -import { IVirtualEnvironmentIdentifier, IVirtualEnvironmentManager } from './virtualEnvs/types'; -import { VEnv } from './virtualEnvs/venv'; -import { VirtualEnv } from './virtualEnvs/virtualEnv'; +import { IVirtualEnvironmentManager } from './virtualEnvs/types'; export function registerTypes(serviceManager: IServiceManager) { serviceManager.addSingletonInstance(IKnownSearchPathsForInterpreters, getKnownSearchPathsForInterpreters()); @@ -51,9 +49,6 @@ export function registerTypes(serviceManager: IServiceManager) { serviceManager.addSingleton(IVirtualEnvironmentsSearchPathProvider, WorkspaceVirtualEnvironmentsSearchPathProvider, 'workspace'); serviceManager.addSingleton(ICondaService, CondaService); - serviceManager.addSingleton(IVirtualEnvironmentIdentifier, VirtualEnv); - serviceManager.addSingleton(IVirtualEnvironmentIdentifier, VEnv); - serviceManager.addSingleton(IVirtualEnvironmentManager, VirtualEnvironmentManager); serviceManager.addSingleton(IInterpreterVersionService, InterpreterVersionService); diff --git a/src/client/interpreter/virtualEnvs/index.ts b/src/client/interpreter/virtualEnvs/index.ts index 01a538fbb012..8798de113ca4 100644 --- a/src/client/interpreter/virtualEnvs/index.ts +++ b/src/client/interpreter/virtualEnvs/index.ts @@ -1,21 +1,26 @@ -import { injectable, multiInject } from 'inversify'; -import { IVirtualEnvironmentIdentifier, IVirtualEnvironmentManager } from './types'; +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { inject, injectable } from 'inversify'; +import { IProcessService } from '../../common/process/types'; +import { IServiceContainer } from '../../ioc/types'; +import { IVirtualEnvironmentManager } from './types'; @injectable() export class VirtualEnvironmentManager implements IVirtualEnvironmentManager { - constructor(@multiInject(IVirtualEnvironmentIdentifier) private envs: IVirtualEnvironmentIdentifier[]) { + private processService: IProcessService; + constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) { + this.processService = serviceContainer.get(IProcessService); } - public detect(pythonPath: string): Promise { - const promises = this.envs - .map(item => item.detect(pythonPath) - .then(result => { - return { env: item, result }; - })); - - return Promise.all(promises) - .then(results => { - const env = results.find(items => items.result === true); - return env ? env.env : undefined; - }); + public async detect(pythonPath: string): Promise { + // https://stackoverflow.com/questions/1871549/determine-if-python-is-running-inside-virtualenv + const output = await this.processService.exec(pythonPath, ['-c', 'import sys;print(hasattr(sys, "real_prefix"))']); + if (output.stdout.length > 0) { + const result = output.stdout.trim(); + if (result === 'True') { + return 'virtualenv'; + } + } + return ''; } } diff --git a/src/client/interpreter/virtualEnvs/types.ts b/src/client/interpreter/virtualEnvs/types.ts index 10c44f9e8e96..3ecdc18f36e5 100644 --- a/src/client/interpreter/virtualEnvs/types.ts +++ b/src/client/interpreter/virtualEnvs/types.ts @@ -1,15 +1,7 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -import { InterpreterType } from '../contracts'; -export const IVirtualEnvironmentIdentifier = Symbol('IVirtualEnvironment'); - -export interface IVirtualEnvironmentIdentifier { - readonly name: string; - readonly type: InterpreterType.VEnv | InterpreterType.VirtualEnv; - detect(pythonPath: string): Promise; -} export const IVirtualEnvironmentManager = Symbol('VirtualEnvironmentManager'); export interface IVirtualEnvironmentManager { - detect(pythonPath: string): Promise; + detect(pythonPath: string): Promise; } diff --git a/src/client/interpreter/virtualEnvs/venv.ts b/src/client/interpreter/virtualEnvs/venv.ts deleted file mode 100644 index 3fca7bb4f2c0..000000000000 --- a/src/client/interpreter/virtualEnvs/venv.ts +++ /dev/null @@ -1,27 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -import { inject, injectable } from 'inversify'; -import * as path from 'path'; -import { IFileSystem } from '../../common/platform/types'; -import { IServiceContainer } from '../../ioc/types'; -import { InterpreterType } from '../contracts'; -import { IVirtualEnvironmentIdentifier } from './types'; - -const pyEnvCfgFileName = 'pyvenv.cfg'; - -@injectable() -export class VEnv implements IVirtualEnvironmentIdentifier { - public readonly name: string = 'venv'; - public readonly type = InterpreterType.VEnv; - private fs: IFileSystem; - - constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) { - this.fs = serviceContainer.get(IFileSystem); - } - public detect(pythonPath: string): Promise { - const dir = path.dirname(pythonPath); - const pyEnvCfgPath = path.join(dir, '..', pyEnvCfgFileName); - return this.fs.fileExistsAsync(pyEnvCfgPath); - } -} diff --git a/src/client/interpreter/virtualEnvs/virtualEnv.ts b/src/client/interpreter/virtualEnvs/virtualEnv.ts deleted file mode 100644 index e1dada31a344..000000000000 --- a/src/client/interpreter/virtualEnvs/virtualEnv.ts +++ /dev/null @@ -1,27 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -import { inject, injectable } from 'inversify'; -import * as path from 'path'; -import { IFileSystem } from '../../common/platform/types'; -import { IServiceContainer } from '../../ioc/types'; -import { InterpreterType } from '../contracts'; -import { IVirtualEnvironmentIdentifier } from './types'; - -@injectable() -export class VirtualEnv implements IVirtualEnvironmentIdentifier { - public readonly name: string = 'virtualenv'; - public readonly type = InterpreterType.VirtualEnv; - private fs: IFileSystem; - - constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) { - this.fs = serviceContainer.get(IFileSystem); - } - public async detect(pythonPath: string): Promise { - const dir = path.dirname(pythonPath); - const libExists = await this.fs.directoryExistsAsync(path.join(dir, '..', 'lib')); - const binExists = await this.fs.directoryExistsAsync(path.join(dir, '..', 'bin')); - const includeExists = await this.fs.directoryExistsAsync(path.join(dir, '..', 'include')); - return libExists && binExists && includeExists; - } -} diff --git a/src/test/interpreters/display.test.ts b/src/test/interpreters/display.test.ts index e83ad98ee6cd..57afc8890cf0 100644 --- a/src/test/interpreters/display.test.ts +++ b/src/test/interpreters/display.test.ts @@ -113,7 +113,7 @@ suite('Interpreters Display', () => { interpreterService.setup(i => i.getActiveInterpreter(TypeMoq.It.isValue(workspaceFolder))).returns(() => Promise.resolve(undefined)); configurationService.setup(c => c.getSettings(TypeMoq.It.isAny())).returns(() => pythonSettings.object); pythonSettings.setup(p => p.pythonPath).returns(() => pythonPath); - virtualEnvMgr.setup(v => v.detect(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve(undefined)); + virtualEnvMgr.setup(v => v.detect(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve('')); versionProvider.setup(v => v.getVersion(TypeMoq.It.isValue(pythonPath), TypeMoq.It.isAny())).returns((_path, defaultDisplayName) => Promise.resolve(defaultDisplayName)); await interpreterDisplay.refresh(resource); @@ -152,7 +152,7 @@ suite('Interpreters Display', () => { fileSystem.setup(f => f.fileExistsAsync(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve(false)); const defaultDisplayName = `${path.basename(pythonPath)} [Environment]`; versionProvider.setup(v => v.getVersion(TypeMoq.It.isValue(pythonPath), TypeMoq.It.isAny())).returns(() => Promise.resolve(defaultDisplayName)); - virtualEnvMgr.setup(v => v.detect(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve(undefined)); + virtualEnvMgr.setup(v => v.detect(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve('')); await interpreterDisplay.refresh(resource); @@ -194,7 +194,7 @@ suite('Interpreters Display', () => { const displayName = 'Version from Interperter'; versionProvider.setup(v => v.getVersion(TypeMoq.It.isValue(pythonPath), TypeMoq.It.isAny())).returns(() => Promise.resolve(displayName)); // tslint:disable-next-line:no-any - virtualEnvMgr.setup(v => v.detect(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve(undefined)); + virtualEnvMgr.setup(v => v.detect(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve('')); await interpreterDisplay.refresh(resource); diff --git a/src/test/interpreters/mocks.ts b/src/test/interpreters/mocks.ts index 3919ebfb407f..2c79853a15ce 100644 --- a/src/test/interpreters/mocks.ts +++ b/src/test/interpreters/mocks.ts @@ -1,8 +1,7 @@ import { injectable } from 'inversify'; import { Architecture, IRegistry, RegistryHive } from '../../client/common/platform/types'; import { IPersistentState } from '../../client/common/types'; -import { IInterpreterVersionService, InterpreterType } from '../../client/interpreter/contracts'; -import { IVirtualEnvironmentIdentifier } from '../../client/interpreter/virtualEnvs/types'; +import { IInterpreterVersionService } from '../../client/interpreter/contracts'; @injectable() export class MockRegistry implements IRegistry { @@ -37,15 +36,6 @@ export class MockRegistry implements IRegistry { } } -@injectable() -export class MockVirtualEnv implements IVirtualEnvironmentIdentifier { - constructor(private isDetected: boolean, public name: string, public type: InterpreterType.VirtualEnv | InterpreterType.VEnv = InterpreterType.VirtualEnv) { - } - public async detect(pythonPath: string): Promise { - return Promise.resolve(this.isDetected); - } -} - // tslint:disable-next-line:max-classes-per-file @injectable() export class MockInterpreterVersionProvider implements IInterpreterVersionService { From ec563c760cd21db66d9f6a5a2f2304cab60243f5 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Sat, 17 Feb 2018 14:52:31 -0800 Subject: [PATCH 17/32] Remove duplicates --- src/client/common/platform/fileSystem.ts | 7 ++++++ src/client/common/platform/types.ts | 1 + .../configuration/interpreterSelector.ts | 23 ++++++++++++++++++- 3 files changed, 30 insertions(+), 1 deletion(-) diff --git a/src/client/common/platform/fileSystem.ts b/src/client/common/platform/fileSystem.ts index 794d9efc04d6..89ec9aac3e9e 100644 --- a/src/client/common/platform/fileSystem.ts +++ b/src/client/common/platform/fileSystem.ts @@ -86,4 +86,11 @@ export class FileSystem implements IFileSystem { return fs.appendFileSync(filename, data, optionsOrEncoding); } + public getRealPathAsync(filePath: string): Promise { + return new Promise(resolve => { + fs.realpath(filePath, (err, realPath) => { + resolve(err ? filePath : realPath); + }); + }); + } } diff --git a/src/client/common/platform/types.ts b/src/client/common/platform/types.ts index 15f1bae1bdfa..b78c8886f91c 100644 --- a/src/client/common/platform/types.ts +++ b/src/client/common/platform/types.ts @@ -41,4 +41,5 @@ export interface IFileSystem { appendFileSync(filename: string, data: {}, options?: { encoding?: string; mode?: number; flag?: string; }): void; // tslint:disable-next-line:unified-signatures appendFileSync(filename: string, data: {}, options?: { encoding?: string; mode?: string; flag?: string; }): void; + getRealPathAsync(path: string): Promise; } diff --git a/src/client/interpreter/configuration/interpreterSelector.ts b/src/client/interpreter/configuration/interpreterSelector.ts index 6dc9dc0c7033..34433ffcc424 100644 --- a/src/client/interpreter/configuration/interpreterSelector.ts +++ b/src/client/interpreter/configuration/interpreterSelector.ts @@ -4,6 +4,7 @@ import { ConfigurationTarget, Disposable, QuickPickItem, QuickPickOptions, Uri } import { IApplicationShell, ICommandManager, IDocumentManager, IWorkspaceService } from '../../common/application/types'; import * as settings from '../../common/configSettings'; import { Commands } from '../../common/constants'; +import { IFileSystem } from '../../common/platform/types'; import { IServiceContainer } from '../../ioc/types'; import { IInterpreterService, IShebangCodeLensProvider, PythonInterpreter, WorkspacePythonPath } from '../contracts'; import { IInterpreterSelector, IPythonPathUpdaterServiceManager } from './types'; @@ -20,11 +21,14 @@ export class InterpreterSelector implements IInterpreterSelector { private readonly workspaceService: IWorkspaceService; private readonly applicationShell: IApplicationShell; private readonly documentManager: IDocumentManager; + private readonly fileSystem: IFileSystem; + constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) { this.interpreterManager = serviceContainer.get(IInterpreterService); this.workspaceService = this.serviceContainer.get(IWorkspaceService); this.applicationShell = this.serviceContainer.get(IApplicationShell); this.documentManager = this.serviceContainer.get(IDocumentManager); + this.fileSystem = this.serviceContainer.get(IFileSystem); const commandManager = serviceContainer.get(ICommandManager); this.disposables.push(commandManager.registerCommand(Commands.Set_Interpreter, this.setInterpreter.bind(this))); @@ -63,12 +67,29 @@ export class InterpreterSelector implements IInterpreterSelector { } private async getSuggestions(resourceUri?: Uri) { - const interpreters = await this.interpreterManager.getInterpreters(resourceUri); + let interpreters = await this.interpreterManager.getInterpreters(resourceUri); + interpreters = await this.removeDuplicates(interpreters); // tslint:disable-next-line:no-non-null-assertion interpreters.sort((a, b) => a.displayName! > b.displayName! ? 1 : -1); return Promise.all(interpreters.map(item => this.suggestionToQuickPickItem(item, resourceUri))); } + private async removeDuplicates(interpreters: PythonInterpreter[]): Promise { + const result: PythonInterpreter[] = []; + for (const i of interpreters) { + const index = result.findIndex(x => x.displayName === i.displayName); + if (index >= 0) { + const existing = interpreters[index]; + const realPath = await this.fileSystem.getRealPathAsync(i.path); + if (this.fileSystem.arePathsSame(realPath, existing.path)) { + continue; + } + } + result.push(i); + } + return result; + } + private async setInterpreter() { const setInterpreterGlobally = !Array.isArray(this.workspaceService.workspaceFolders) || this.workspaceService.workspaceFolders.length === 0; let configTarget = ConfigurationTarget.Global; From 2ad4475bbe232130d9029ea58faea59b43599b40 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Sat, 17 Feb 2018 19:58:50 -0800 Subject: [PATCH 18/32] Test --- .../configuration/interpreterSelector.ts | 51 ++++----- src/client/interpreter/contracts.ts | 1 + .../configuration/interpreterSelector.test.ts | 101 ++++++++++++++++++ 3 files changed, 128 insertions(+), 25 deletions(-) create mode 100644 src/test/configuration/interpreterSelector.test.ts diff --git a/src/client/interpreter/configuration/interpreterSelector.ts b/src/client/interpreter/configuration/interpreterSelector.ts index 34433ffcc424..607be98c174c 100644 --- a/src/client/interpreter/configuration/interpreterSelector.ts +++ b/src/client/interpreter/configuration/interpreterSelector.ts @@ -9,14 +9,13 @@ import { IServiceContainer } from '../../ioc/types'; import { IInterpreterService, IShebangCodeLensProvider, PythonInterpreter, WorkspacePythonPath } from '../contracts'; import { IInterpreterSelector, IPythonPathUpdaterServiceManager } from './types'; -interface IInterpreterQuickPickItem extends QuickPickItem { +export interface IInterpreterQuickPickItem extends QuickPickItem { path: string; } @injectable() export class InterpreterSelector implements IInterpreterSelector { private disposables: Disposable[] = []; - private pythonPathUpdaterService: IPythonPathUpdaterServiceManager; private readonly interpreterManager: IInterpreterService; private readonly workspaceService: IWorkspaceService; private readonly applicationShell: IApplicationShell; @@ -33,11 +32,19 @@ export class InterpreterSelector implements IInterpreterSelector { const commandManager = serviceContainer.get(ICommandManager); this.disposables.push(commandManager.registerCommand(Commands.Set_Interpreter, this.setInterpreter.bind(this))); this.disposables.push(commandManager.registerCommand(Commands.Set_ShebangInterpreter, this.setShebangInterpreter.bind(this))); - this.pythonPathUpdaterService = serviceContainer.get(IPythonPathUpdaterServiceManager); } public dispose() { this.disposables.forEach(disposable => disposable.dispose()); } + + public async getSuggestions(resourceUri?: Uri) { + let interpreters = await this.interpreterManager.getInterpreters(resourceUri); + interpreters = await this.removeDuplicates(interpreters); + // tslint:disable-next-line:no-non-null-assertion + interpreters.sort((a, b) => a.displayName! > b.displayName! ? 1 : -1); + return Promise.all(interpreters.map(item => this.suggestionToQuickPickItem(item, resourceUri))); + } + private async getWorkspaceToSetPythonPath(): Promise { if (!Array.isArray(this.workspaceService.workspaceFolders) || this.workspaceService.workspaceFolders.length === 0) { return undefined; @@ -51,6 +58,7 @@ export class InterpreterSelector implements IInterpreterSelector { const workspaceFolder = await applicationShell.showWorkspaceFolderPick({ placeHolder: 'Select a workspace' }); return workspaceFolder ? { folderUri: workspaceFolder.uri, configTarget: ConfigurationTarget.WorkspaceFolder } : undefined; } + private async suggestionToQuickPickItem(suggestion: PythonInterpreter, workspaceUri?: Uri): Promise { let detail = suggestion.path; if (workspaceUri && suggestion.path.startsWith(workspaceUri.fsPath)) { @@ -66,27 +74,18 @@ export class InterpreterSelector implements IInterpreterSelector { }; } - private async getSuggestions(resourceUri?: Uri) { - let interpreters = await this.interpreterManager.getInterpreters(resourceUri); - interpreters = await this.removeDuplicates(interpreters); - // tslint:disable-next-line:no-non-null-assertion - interpreters.sort((a, b) => a.displayName! > b.displayName! ? 1 : -1); - return Promise.all(interpreters.map(item => this.suggestionToQuickPickItem(item, resourceUri))); - } - private async removeDuplicates(interpreters: PythonInterpreter[]): Promise { const result: PythonInterpreter[] = []; - for (const i of interpreters) { - const index = result.findIndex(x => x.displayName === i.displayName); - if (index >= 0) { - const existing = interpreters[index]; - const realPath = await this.fileSystem.getRealPathAsync(i.path); - if (this.fileSystem.arePathsSame(realPath, existing.path)) { - continue; - } + await Promise.all(interpreters.filter(async x => { + x.realPath = await this.fileSystem.getRealPathAsync(x.path); + return true; + })); + interpreters.forEach(x => { + if (result.findIndex(a => a.displayName === x.displayName + && a.type === x.type && this.fileSystem.arePathsSame(a.realPath!, x.realPath!)) < 0) { + result.push(x); } - result.push(i); - } + }); return result; } @@ -116,7 +115,8 @@ export class InterpreterSelector implements IInterpreterSelector { const selection = await this.applicationShell.showQuickPick(suggestions, quickPickOptions); if (selection !== undefined) { - await this.pythonPathUpdaterService.updatePythonPath(selection.path, configTarget, 'ui', wkspace); + const pythonPathUpdaterService = this.serviceContainer.get(IPythonPathUpdaterServiceManager); + await pythonPathUpdaterService.updatePythonPath(selection.path, configTarget, 'ui', wkspace); } } @@ -131,16 +131,17 @@ export class InterpreterSelector implements IInterpreterSelector { const workspaceFolder = this.workspaceService.getWorkspaceFolder(this.documentManager.activeTextEditor!.document.uri); const isWorkspaceChange = Array.isArray(this.workspaceService.workspaceFolders) && this.workspaceService.workspaceFolders.length === 1; + const pythonPathUpdaterService = this.serviceContainer.get(IPythonPathUpdaterServiceManager); if (isGlobalChange) { - await this.pythonPathUpdaterService.updatePythonPath(shebang, ConfigurationTarget.Global, 'shebang'); + await pythonPathUpdaterService.updatePythonPath(shebang, ConfigurationTarget.Global, 'shebang'); return; } if (isWorkspaceChange || !workspaceFolder) { - await this.pythonPathUpdaterService.updatePythonPath(shebang, ConfigurationTarget.Workspace, 'shebang', this.workspaceService.workspaceFolders![0].uri); + await pythonPathUpdaterService.updatePythonPath(shebang, ConfigurationTarget.Workspace, 'shebang', this.workspaceService.workspaceFolders![0].uri); return; } - await this.pythonPathUpdaterService.updatePythonPath(shebang, ConfigurationTarget.WorkspaceFolder, 'shebang', workspaceFolder.uri); + await pythonPathUpdaterService.updatePythonPath(shebang, ConfigurationTarget.WorkspaceFolder, 'shebang', workspaceFolder.uri); } } diff --git a/src/client/interpreter/contracts.ts b/src/client/interpreter/contracts.ts index fc3138bf1170..040c910c7373 100644 --- a/src/client/interpreter/contracts.ts +++ b/src/client/interpreter/contracts.ts @@ -66,6 +66,7 @@ export type PythonInterpreter = { envName?: string; envPath?: string; cachedEntry?: boolean; + realPath?: string; }; export type WorkspacePythonPath = { diff --git a/src/test/configuration/interpreterSelector.test.ts b/src/test/configuration/interpreterSelector.test.ts new file mode 100644 index 000000000000..b53dd61c79d2 --- /dev/null +++ b/src/test/configuration/interpreterSelector.test.ts @@ -0,0 +1,101 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import * as assert from 'assert'; +import { Container } from 'inversify'; +import * as TypeMoq from 'typemoq'; +import { IApplicationShell, ICommandManager, IDocumentManager, IWorkspaceService } from '../../client/common/application/types'; +import { IFileSystem } from '../../client/common/platform/types'; +import { IInterpreterQuickPickItem, InterpreterSelector } from '../../client/interpreter/configuration/interpreterSelector'; +import { IInterpreterService, InterpreterType, PythonInterpreter } from '../../client/interpreter/contracts'; +import { ServiceContainer } from '../../client/ioc/container'; +import { ServiceManager } from '../../client/ioc/serviceManager'; +import { IServiceContainer } from '../../client/ioc/types'; + +class InterpreterQuickPickItem implements IInterpreterQuickPickItem { + public path: string; + public label: string; + public description: string; + public detail?: string; + constructor(l: string, p: string) { + this.path = p; + this.label = l; + } +} + +// tslint:disable-next-line:max-func-body-length +suite('Intepreters - selector', () => { + let serviceContainer: IServiceContainer; + let workspace: TypeMoq.IMock; + let appShell: TypeMoq.IMock; + let interpreterService: TypeMoq.IMock; + let documentManager: TypeMoq.IMock; + let fileSystem: TypeMoq.IMock; + + setup(() => { + const cont = new Container(); + const serviceManager = new ServiceManager(cont); + serviceContainer = new ServiceContainer(cont); + + workspace = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(IWorkspaceService, workspace.object); + + appShell = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(IApplicationShell, appShell.object); + + interpreterService = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(IInterpreterService, interpreterService.object); + + documentManager = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(IDocumentManager, documentManager.object); + + fileSystem = TypeMoq.Mock.ofType(); + fileSystem + .setup(x => x.arePathsSame(TypeMoq.It.isAnyString(), TypeMoq.It.isAnyString())) + .returns((a: string, b: string) => a === b); + fileSystem + .setup(x => x.getRealPathAsync(TypeMoq.It.isAnyString())) + .returns((a: string) => new Promise(resolve => resolve(a))); + + serviceManager.addSingletonInstance(IFileSystem, fileSystem.object); + + const commandManager = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(ICommandManager, commandManager.object); + }); + + test('Suggestions', async () => { + const initial: PythonInterpreter[] = [ + { displayName: '1', path: 'path1', type: InterpreterType.Unknown }, + { displayName: '2', path: 'path1', type: InterpreterType.Unknown }, + { displayName: '1', path: 'path1', type: InterpreterType.Unknown }, + { displayName: '2', path: 'path2', type: InterpreterType.Unknown }, + { displayName: '2', path: 'path2', type: InterpreterType.Unknown }, + { displayName: '2 (virtualenv)', path: 'path2', type: InterpreterType.VirtualEnv }, + { displayName: '3', path: 'path2', type: InterpreterType.Unknown }, + { displayName: '4', path: 'path4', type: InterpreterType.Conda } + ]; + interpreterService + .setup(x => x.getInterpreters(TypeMoq.It.isAny())) + .returns(() => new Promise((resolve) => resolve(initial))); + + const selector = new InterpreterSelector(serviceContainer); + const actual = await selector.getSuggestions(); + + const expected: InterpreterQuickPickItem[] = [ + new InterpreterQuickPickItem('1', 'path1'), + new InterpreterQuickPickItem('2', 'path1'), + new InterpreterQuickPickItem('2', 'path2'), + new InterpreterQuickPickItem('2 (virtualenv)', 'path2'), + new InterpreterQuickPickItem('3', 'path2'), + new InterpreterQuickPickItem('4', 'path4') + ]; + + assert.equal(actual.length, expected.length, 'Suggestion lengths are different.'); + for (let i = 0; i < expected.length; i += 1) { + assert.equal(actual[i].label, expected[i].label, + `Suggestion label is different at ${i}: exected '${expected[i].label}', found '${actual[i].label}'.`); + assert.equal(actual[i].path, expected[i].path, + `Suggestion path is different at ${i}: exected '${expected[i].path}', found '${actual[i].path}'.`); + } + }); +}); From 1ee0be28809f52ad3e41c8a9f1e8b0c5e7700b49 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Sun, 18 Feb 2018 22:51:04 -0800 Subject: [PATCH 19/32] Discover pylintrc better + tests --- src/client/linters/baseLinter.ts | 18 ++--- src/client/linters/pylint.ts | 29 ++++++-- src/test/linters/pylint.test.ts | 118 ++++++++++++++++++++++++++++++- src/test/mockClasses.ts | 37 ++++++++++ 4 files changed, 187 insertions(+), 15 deletions(-) diff --git a/src/client/linters/baseLinter.ts b/src/client/linters/baseLinter.ts index 2c90e5c42731..742008ea7346 100644 --- a/src/client/linters/baseLinter.ts +++ b/src/client/linters/baseLinter.ts @@ -1,10 +1,10 @@ import * as path from 'path'; import * as vscode from 'vscode'; -import { CancellationToken, OutputChannel, TextDocument, Uri } from 'vscode'; +import { IWorkspaceService } from '../common/application/types'; import '../common/extensions'; import { IPythonToolExecutionService } from '../common/process/types'; -import { ExecutionInfo, ILogger, Product } from '../common/types'; import { IConfigurationService, IPythonSettings } from '../common/types'; +import { ExecutionInfo, ILogger, Product } from '../common/types'; import { IServiceContainer } from '../ioc/types'; import { ErrorHandler } from './errorHandlers/errorHandler'; import { ILinter, ILinterInfo, ILinterManager, ILintMessage, LintMessageSeverity } from './types'; @@ -38,25 +38,27 @@ export abstract class BaseLinter implements ILinter { private errorHandler: ErrorHandler; private _pythonSettings: IPythonSettings; private _info: ILinterInfo; + private workspace: IWorkspaceService; protected get pythonSettings(): IPythonSettings { return this._pythonSettings; } constructor(product: Product, - protected readonly outputChannel: OutputChannel, + protected readonly outputChannel: vscode.OutputChannel, protected readonly serviceContainer: IServiceContainer, protected readonly columnOffset = 0) { this._info = serviceContainer.get(ILinterManager).getLinterInfo(product); this.errorHandler = new ErrorHandler(this.info.product, outputChannel, serviceContainer); this.configService = serviceContainer.get(IConfigurationService); + this.workspace = serviceContainer.get(IWorkspaceService); } public get info(): ILinterInfo { return this._info; } - public isLinterExecutableSpecified(resource: Uri) { + public isLinterExecutableSpecified(resource: vscode.Uri) { const executablePath = this.info.pathName(resource); return path.basename(executablePath).length > 0 && path.basename(executablePath) !== executablePath; } @@ -66,7 +68,7 @@ export abstract class BaseLinter implements ILinter { } protected getWorkspaceRootPath(document: vscode.TextDocument): string { - const workspaceFolder = vscode.workspace.getWorkspaceFolder(document.uri); + const workspaceFolder = this.workspace.getWorkspaceFolder(document.uri); const workspaceRootPath = (workspaceFolder && typeof workspaceFolder.uri.fsPath === 'string') ? workspaceFolder.uri.fsPath : undefined; return typeof workspaceRootPath === 'string' ? workspaceRootPath : __dirname; } @@ -107,7 +109,7 @@ export abstract class BaseLinter implements ILinter { const cwd = this.getWorkspaceRootPath(document); const pythonToolsExecutionService = this.serviceContainer.get(IPythonToolExecutionService); try { - const result = await pythonToolsExecutionService.exec(executionInfo, {cwd, token: cancellation, mergeStdOutErr: true}, document.uri); + const result = await pythonToolsExecutionService.exec(executionInfo, { cwd, token: cancellation, mergeStdOutErr: true }, document.uri); this.displayLinterResultHeader(result.stdout); return await this.parseMessages(result.stdout, document, cancellation, regEx); } catch (error) { @@ -116,12 +118,12 @@ export abstract class BaseLinter implements ILinter { } } - protected async parseMessages(output: string, document: TextDocument, token: CancellationToken, regEx: string) { + protected async parseMessages(output: string, document: vscode.TextDocument, token: vscode.CancellationToken, regEx: string) { const outputLines = output.splitLines({ removeEmptyEntries: false, trim: false }); return this.parseLines(outputLines, regEx); } - protected handleError(error: Error, resource: Uri, execInfo: ExecutionInfo) { + protected handleError(error: Error, resource: vscode.Uri, execInfo: ExecutionInfo) { this.errorHandler.handleError(error, resource, execInfo) .catch(this.logger.logError.bind(this, 'Error in errorHandler.handleError')); } diff --git a/src/client/linters/pylint.ts b/src/client/linters/pylint.ts index c6f6e466ba10..53caae8a8669 100644 --- a/src/client/linters/pylint.ts +++ b/src/client/linters/pylint.ts @@ -11,6 +11,9 @@ import { IServiceContainer } from '../ioc/types'; import { BaseLinter } from './baseLinter'; import { ILintMessage } from './types'; +const pylintrc = 'pylintrc'; +const dotPylintrc = '.pylintrc'; + export class Pylint extends BaseLinter { private fileSystem: IFileSystem; private platformService: IPlatformService; @@ -27,12 +30,13 @@ export class Pylint extends BaseLinter { // a) there are no custom arguments and // b) there is no pylintrc file next to the file or at the workspace root const uri = document.uri; + const workspaceRoot = this.getWorkspaceRootPath(document); const settings = this.configService.getSettings(uri); if (settings.linting.pylintUseMinimalCheckers && this.info.linterArgs(uri).length === 0 - // Check pylintrc next to the file - && !await Pylint.hasConfigurationFile(this.fileSystem, path.dirname(uri.fsPath), this.platformService) - // Checn for pylintrc at the root (function will strip the file name) + // Check pylintrc next to the file or above up to and including the workspace root + && !await Pylint.hasConfigrationFileInWorkspace(this.fileSystem, path.dirname(uri.fsPath), workspaceRoot) + // Check for pylintrc at the root and above && !await Pylint.hasConfigurationFile(this.fileSystem, this.getWorkspaceRootPath(document), this.platformService)) { minArgs = [ '--disable=all', @@ -73,8 +77,6 @@ export class Pylint extends BaseLinter { } let dir = folder; - const pylintrc = 'pylintrc'; - const dotPylintrc = '.pylintrc'; if (await fs.fileExistsAsync(path.join(dir, pylintrc)) || await fs.fileExistsAsync(path.join(dir, dotPylintrc))) { return true; } @@ -90,7 +92,7 @@ export class Pylint extends BaseLinter { } current = above; above = path.dirname(above); - } while (current !== above); + } while (!fs.arePathsSame(current, above)); dir = path.resolve('~'); if (await fs.fileExistsAsync(path.join(dir, dotPylintrc))) { @@ -107,4 +109,19 @@ export class Pylint extends BaseLinter { } return false; } + + // tslint:disable-next-line:member-ordering + public static async hasConfigrationFileInWorkspace(fs: IFileSystem, folder: string, root: string): Promise { + // Search up from file location to the workspace root + let current = folder; + let above = path.dirname(current); + do { + if (await fs.fileExistsAsync(path.join(current, pylintrc)) || await fs.fileExistsAsync(path.join(current, dotPylintrc))) { + return true; + } + current = above; + above = path.dirname(above); + } while (!fs.arePathsSame(current, root) && !fs.arePathsSame(current, above)); + return false; + } } diff --git a/src/test/linters/pylint.test.ts b/src/test/linters/pylint.test.ts index 5ed565b2d44f..75cd78f148f3 100644 --- a/src/test/linters/pylint.test.ts +++ b/src/test/linters/pylint.test.ts @@ -2,11 +2,22 @@ // Licensed under the MIT License. import { expect } from 'chai'; +import { Container } from 'inversify'; import * as path from 'path'; import * as TypeMoq from 'typemoq'; +import { CancellationTokenSource, OutputChannel, TextDocument, Uri, WorkspaceFolder } from 'vscode'; +import { IWorkspaceService } from '../../client/common/application/types'; import { IFileSystem, IPlatformService } from '../../client/common/platform/types'; +import { IPythonToolExecutionService } from '../../client/common/process/types'; +import { ExecutionInfo, IConfigurationService, IInstaller, ILogger, IPythonSettings } from '../../client/common/types'; +import { ServiceContainer } from '../../client/ioc/container'; +import { ServiceManager } from '../../client/ioc/serviceManager'; +import { LinterManager } from '../../client/linters/linterManager'; import { Pylint } from '../../client/linters/pylint'; +import { ILinterManager } from '../../client/linters/types'; +import { MockLintingSettings } from '../mockClasses'; +// tslint:disable-next-line:max-func-body-length suite('Linting - Pylintrc search', () => { const basePath = '/user/a/b/c/d'; const pylintrc = 'pylintrc'; @@ -14,10 +25,40 @@ suite('Linting - Pylintrc search', () => { let fileSystem: TypeMoq.IMock; let platformService: TypeMoq.IMock; + let workspace: TypeMoq.IMock; + let execService: TypeMoq.IMock; + let config: TypeMoq.IMock; + let serviceContainer: ServiceContainer; setup(() => { fileSystem = TypeMoq.Mock.ofType(); + fileSystem + .setup(x => x.arePathsSame(TypeMoq.It.isAnyString(), TypeMoq.It.isAnyString())) + .returns((a, b) => a === b); + platformService = TypeMoq.Mock.ofType(); + platformService.setup(x => x.isWindows).returns(() => false); + + workspace = TypeMoq.Mock.ofType(); + execService = TypeMoq.Mock.ofType(); + + const cont = new Container(); + const serviceManager = new ServiceManager(cont); + serviceContainer = new ServiceContainer(cont); + + serviceManager.addSingletonInstance(IFileSystem, fileSystem.object); + serviceManager.addSingletonInstance(IWorkspaceService, workspace.object); + serviceManager.addSingletonInstance(IPythonToolExecutionService, execService.object); + serviceManager.addSingletonInstance(IPlatformService, platformService.object); + + config = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(IConfigurationService, config.object); + const linterManager = new LinterManager(serviceContainer); + serviceManager.addSingletonInstance(ILinterManager, linterManager); + const logger = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(ILogger, logger.object); + const installer = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(IInstaller, installer.object); }); test('pylintrc in the file folder', async () => { @@ -75,11 +116,86 @@ suite('Linting - Pylintrc search', () => { expect(result).to.be.equal(true, `'${pylintrc}' not detected in the ~/.config folder.`); }); test('pylintrc in the /etc folder', async () => { - platformService.setup(x => x.isWindows).returns(() => false); const rc = path.join('/etc', pylintrc); 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, `'${pylintrc}' not detected in the /etc folder.`); }); + test('pylintrc between file and workspace root', async () => { + const root = '/user/a'; + const midFolder = '/user/a/b'; + fileSystem + .setup(x => x.fileExistsAsync(path.join(midFolder, pylintrc))) + .returns(() => Promise.resolve(true)); + + const result = await Pylint.hasConfigrationFileInWorkspace(fileSystem.object, basePath, root); + expect(result).to.be.equal(true, `'${pylintrc}' not detected in the workspace tree.`); + }); + + test('minArgs - pylintrc between the file and the workspace root', async () => { + fileSystem + .setup(x => x.fileExistsAsync(path.join('/user/a/b', pylintrc))) + .returns(() => Promise.resolve(true)); + + await testPylintArguments('/user/a/b/c', '/user/a', false); + }); + + test('minArgs - no pylintrc between the file and the workspace root', async () => { + await testPylintArguments('/user/a/b/c', '/user/a', true); + }); + + test('minArgs - pylintrc next to the file', async () => { + const fileFolder = '/user/a/b/c'; + fileSystem + .setup(x => x.fileExistsAsync(path.join(fileFolder, pylintrc))) + .returns(() => Promise.resolve(true)); + + await testPylintArguments(fileFolder, '/user/a', false); + }); + + test('minArgs - pylintrc at the workspace root', async () => { + const root = '/user/a'; + fileSystem + .setup(x => x.fileExistsAsync(path.join(root, pylintrc))) + .returns(() => Promise.resolve(true)); + + await testPylintArguments('/user/a/b/c', root, false); + }); + + async function testPylintArguments(fileFolder: string, wsRoot: string, expectedMinArgs: boolean): Promise { + const outputChannel = TypeMoq.Mock.ofType(); + const pylinter = new Pylint(outputChannel.object, serviceContainer); + + const document = TypeMoq.Mock.ofType(); + document.setup(x => x.uri).returns(() => Uri.file(path.join(fileFolder, 'test.py'))); + + const wsf = TypeMoq.Mock.ofType(); + wsf.setup(x => x.uri).returns(() => Uri.file(wsRoot)); + + workspace.setup(x => x.getWorkspaceFolder(TypeMoq.It.isAny())).returns(() => wsf.object); + + let execInfo: ExecutionInfo | undefined; + execService + .setup(x => x.exec(TypeMoq.It.isAny(), TypeMoq.It.isAny(), TypeMoq.It.isAny())) + .callback((e: ExecutionInfo, b, c) => { + execInfo = e; + }) + .returns(() => Promise.resolve({ stdout: '', stderr: '' })); + + const lintSettings = new MockLintingSettings(); + lintSettings.pylintUseMinimalCheckers = true; + // tslint:disable-next-line:no-string-literal + lintSettings['pylintPath'] = 'pyLint'; + // tslint:disable-next-line:no-string-literal + lintSettings['pylintEnabled'] = true; + + const settings = TypeMoq.Mock.ofType(); + settings.setup(x => x.linting).returns(() => lintSettings); + config.setup(x => x.getSettings(TypeMoq.It.isAny())).returns(() => settings.object); + + await pylinter.lint(document.object, new CancellationTokenSource().token); + expect(execInfo!.args.findIndex(x => x.indexOf('--disable=all') >= 0), + 'Minimal args passed to pylint while pylintrc exists.').to.be.eq(expectedMinArgs ? 0 : -1); + } }); diff --git a/src/test/mockClasses.ts b/src/test/mockClasses.ts index 3cbae4ddfc55..7849e2a2137a 100644 --- a/src/test/mockClasses.ts +++ b/src/test/mockClasses.ts @@ -1,4 +1,8 @@ import * as vscode from 'vscode'; +import { + Flake8CategorySeverity, ILintingSettings, IMypyCategorySeverity, + IPep8CategorySeverity, IPylintCategorySeverity +} from '../client/common/types'; export class MockOutputChannel implements vscode.OutputChannel { public name: string; @@ -44,3 +48,36 @@ export class MockStatusBarItem implements vscode.StatusBarItem { public dispose(): void { } } + +export class MockLintingSettings implements ILintingSettings { + public enabled: boolean; + public ignorePatterns: string[]; + public prospectorEnabled: boolean; + public prospectorArgs: string[]; + public pylintEnabled: boolean; + public pylintArgs: string[]; + public pep8Enabled: boolean; + public pep8Args: string[]; + public pylamaEnabled: boolean; + public pylamaArgs: string[]; + public flake8Enabled: boolean; + public flake8Args: string[]; + public pydocstyleEnabled: boolean; + public pydocstyleArgs: string[]; + public lintOnSave: boolean; + public maxNumberOfProblems: number; + public pylintCategorySeverity: IPylintCategorySeverity; + public pep8CategorySeverity: IPep8CategorySeverity; + public flake8CategorySeverity: Flake8CategorySeverity; + public mypyCategorySeverity: IMypyCategorySeverity; + public prospectorPath: string; + public pylintPath: string; + public pep8Path: string; + public pylamaPath: string; + public flake8Path: string; + public pydocstylePath: string; + public mypyEnabled: boolean; + public mypyArgs: string[]; + public mypyPath: string; + public pylintUseMinimalCheckers: boolean; +} From 04c5733718975c7688078ffc7f84a9930cdc2a82 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Mon, 19 Feb 2018 12:09:51 -0800 Subject: [PATCH 20/32] Undo change --- src/test/initialize.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/initialize.ts b/src/test/initialize.ts index 863d3b021247..5c172a259f4e 100644 --- a/src/test/initialize.ts +++ b/src/test/initialize.ts @@ -63,7 +63,7 @@ function getPythonPath(): string { // tslint:disable-next-line:no-unsafe-any return process.env.TRAVIS_PYTHON_PATH; } - return '/usr/local/bin/python3'; + return 'python'; } function isMultitrootTest() { From 37328a5f2c25eb784b08f8ecc76b32aa15fde2ff Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Mon, 19 Feb 2018 18:33:21 -0800 Subject: [PATCH 21/32] CR feedback --- .../services/baseVirtualEnvService.ts | 20 ++++++++++------- .../locators/services/currentPathService.ts | 22 +++++++++++-------- .../languageServices/jediProxyFactory.ts | 2 +- 3 files changed, 26 insertions(+), 18 deletions(-) diff --git a/src/client/interpreter/locators/services/baseVirtualEnvService.ts b/src/client/interpreter/locators/services/baseVirtualEnvService.ts index f4d2c2149598..d6000631acde 100644 --- a/src/client/interpreter/locators/services/baseVirtualEnvService.ts +++ b/src/client/interpreter/locators/services/baseVirtualEnvService.ts @@ -65,14 +65,18 @@ export class BaseVirtualEnvService extends CacheableLocatorService { })); } private async getVirtualEnvDetails(interpreter: string): Promise { - const displayName = await this.versionProvider.getVersion(interpreter, path.basename(interpreter)); - const virtualEnv = await this.virtualEnvMgr.detect(interpreter); - const virtualEnvSuffix = virtualEnv ? virtualEnv.name : this.getVirtualEnvironmentRootDirectory(interpreter); - return { - displayName: `${displayName} (${virtualEnvSuffix})`.trim(), - path: interpreter, - type: virtualEnv ? virtualEnv.type : InterpreterType.Unknown - }; + return Promise.all([ + this.versionProvider.getVersion(interpreter, path.basename(interpreter)), + this.virtualEnvMgr.detect(interpreter) + ]) + .then(([displayName, virtualEnv]) => { + const virtualEnvSuffix = virtualEnv ? virtualEnv.name : this.getVirtualEnvironmentRootDirectory(interpreter); + return { + displayName: `${displayName} (${virtualEnvSuffix})`.trim(), + path: interpreter, + type: virtualEnv ? virtualEnv.type : InterpreterType.Unknown + }; + }); } private getVirtualEnvironmentRootDirectory(interpreter: string) { // Python interperters are always in a subdirectory of the environment folder. diff --git a/src/client/interpreter/locators/services/currentPathService.ts b/src/client/interpreter/locators/services/currentPathService.ts index e6d349c86600..62374b78859c 100644 --- a/src/client/interpreter/locators/services/currentPathService.ts +++ b/src/client/interpreter/locators/services/currentPathService.ts @@ -34,15 +34,19 @@ export class CurrentPathService extends CacheableLocatorService { // tslint:disable-next-line:promise-function-async .then(interpreters => Promise.all(interpreters.map(interpreter => this.getInterpreterDetails(interpreter)))); } - private async getInterpreterDetails(interpreter: string) { - let displayName = await this.versionProvider.getVersion(interpreter, path.basename(interpreter)); - const virtualEnv = await this.virtualEnvMgr.detect(interpreter); - displayName += virtualEnv ? ` (${virtualEnv.name})` : ''; - return { - displayName, - path: interpreter, - type: virtualEnv ? virtualEnv.type : InterpreterType.Unknown - }; + private async getInterpreterDetails(interpreter: string): Promise { + return Promise.all([ + this.versionProvider.getVersion(interpreter, path.basename(interpreter)), + this.virtualEnvMgr.detect(interpreter) + ]). + then(([displayName, virtualEnv]) => { + displayName += virtualEnv ? ` (${virtualEnv.name})` : ''; + return { + displayName, + path: interpreter, + type: virtualEnv ? virtualEnv.type : InterpreterType.Unknown + }; + }); } private async getInterpreter(pythonPath: string, defaultValue: string) { return this.processService.exec(pythonPath, ['-c', 'import sys;print(sys.executable)'], {}) diff --git a/src/client/languageServices/jediProxyFactory.ts b/src/client/languageServices/jediProxyFactory.ts index 569e57e66a75..5e18b2396e51 100644 --- a/src/client/languageServices/jediProxyFactory.ts +++ b/src/client/languageServices/jediProxyFactory.ts @@ -15,7 +15,7 @@ export class JediFactory implements Disposable { this.disposables.forEach(disposable => disposable.dispose()); this.disposables = []; } - public getJediProxyHandler(resource: Uri): JediProxyHandler { + public getJediProxyHandler(resource?: Uri): JediProxyHandler { const workspaceFolder = resource ? workspace.getWorkspaceFolder(resource) : undefined; let workspacePath = workspaceFolder ? workspaceFolder.uri.fsPath : undefined; if (!workspacePath) { From 55ff4e54b292d78ab5d9aba7fbe169bdc37736d1 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Mon, 19 Feb 2018 21:24:40 -0800 Subject: [PATCH 22/32] Set interprereter before checking install --- src/client/extension.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/client/extension.ts b/src/client/extension.ts index 21657960b7ca..4c3cb509d6de 100644 --- a/src/client/extension.ts +++ b/src/client/extension.ts @@ -103,14 +103,14 @@ export async function activate(context: vscode.ExtensionContext) { sortImports.activate(context, standardOutputChannel, serviceContainer); const interpreterManager = serviceContainer.get(IInterpreterService); + // This must be completed before we can continue. + interpreterManager.initialize(); + await interpreterManager.autoSetInterpreter(); + const pythonInstaller = new PythonInstaller(serviceContainer); pythonInstaller.checkPythonInstallation(PythonSettings.getInstance()) .catch(ex => console.error('Python Extension: pythonInstaller.checkPythonInstallation', ex)); - // This must be completed before we can continue. - await interpreterManager.autoSetInterpreter(); - - interpreterManager.initialize(); interpreterManager.refresh() .catch(ex => console.error('Python Extension: interpreterManager.refresh', ex)); From 436e5a9f328021afaf9a094c13266aeee0d649d6 Mon Sep 17 00:00:00 2001 From: MikhailArkhipov Date: Tue, 20 Feb 2018 11:01:00 -0800 Subject: [PATCH 23/32] Fix typo and path compare on Windows --- .../debugger/configProvider/provider.test.ts | 26 +++++++++++++------ src/test/interpreters/display.test.ts | 8 +++--- 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/src/test/debugger/configProvider/provider.test.ts b/src/test/debugger/configProvider/provider.test.ts index 6f894cca86a2..7940fb268886 100644 --- a/src/test/debugger/configProvider/provider.test.ts +++ b/src/test/debugger/configProvider/provider.test.ts @@ -72,8 +72,10 @@ import { IServiceContainer } from '../../../client/ioc/types'; expect(debugConfig).to.have.property('type', provider.debugType); expect(debugConfig).to.have.property('request', 'launch'); expect(debugConfig).to.have.property('program', pythonFile); - expect(debugConfig).to.have.property('cwd', __dirname); - expect(debugConfig).to.have.property('envFile', path.join(__dirname, '.env')); + expect(debugConfig).to.have.property('cwd'); + expect(debugConfig!.cwd!.toLowerCase()).to.be.equal(__dirname.toLowerCase()); + expect(debugConfig).to.have.property('envFile'); + expect(debugConfig!.envFile!.toLowerCase()).to.be.equal(path.join(__dirname, '.env').toLowerCase()); expect(debugConfig).to.have.property('env'); // tslint:disable-next-line:no-any expect(Object.keys((debugConfig as any).env)).to.have.lengthOf(0); @@ -92,8 +94,10 @@ import { IServiceContainer } from '../../../client/ioc/types'; expect(debugConfig).to.have.property('type', provider.debugType); expect(debugConfig).to.have.property('request', 'launch'); expect(debugConfig).to.have.property('program', pythonFile); - expect(debugConfig).to.have.property('cwd', __dirname); - expect(debugConfig).to.have.property('envFile', path.join(__dirname, '.env')); + expect(debugConfig).to.have.property('cwd'); + expect(debugConfig!.cwd!.toLowerCase()).to.be.equal(__dirname.toLowerCase()); + expect(debugConfig).to.have.property('envFile'); + expect(debugConfig!.envFile!.toLowerCase()).to.be.equal(path.join(__dirname, '.env').toLowerCase()); expect(debugConfig).to.have.property('env'); // tslint:disable-next-line:no-any expect(Object.keys((debugConfig as any).env)).to.have.lengthOf(0); @@ -106,14 +110,17 @@ import { IServiceContainer } from '../../../client/ioc/types'; setupWorkspaces([]); const debugConfig = await debugProvider.resolveDebugConfiguration!(undefined, {} as DebugConfiguration); + const filePath = Uri.file(path.dirname('')).fsPath; expect(Object.keys(debugConfig!)).to.have.lengthOf.above(3); expect(debugConfig).to.have.property('pythonPath', pythonPath); expect(debugConfig).to.have.property('type', provider.debugType); expect(debugConfig).to.have.property('request', 'launch'); expect(debugConfig).to.have.property('program', pythonFile); - expect(debugConfig).to.have.property('cwd', Uri.file(path.dirname('')).fsPath); - expect(debugConfig).to.have.property('envFile', path.join(Uri.file(path.dirname('')).fsPath, '.env')); + expect(debugConfig).to.have.property('cwd'); + expect(debugConfig!.cwd!.toLowerCase()).to.be.equal(filePath.toLowerCase()); + expect(debugConfig).to.have.property('envFile'); + expect(debugConfig!.envFile!.toLowerCase()).to.be.equal(path.join(filePath, '.env').toLowerCase()); expect(debugConfig).to.have.property('env'); // tslint:disable-next-line:no-any expect(Object.keys((debugConfig as any).env)).to.have.lengthOf(0); @@ -166,14 +173,17 @@ import { IServiceContainer } from '../../../client/ioc/types'; setupWorkspaces([defaultWorkspace]); const debugConfig = await debugProvider.resolveDebugConfiguration!(undefined, {} as DebugConfiguration); + const filePath = Uri.file(defaultWorkspace).fsPath; expect(Object.keys(debugConfig!)).to.have.lengthOf.above(3); expect(debugConfig).to.have.property('pythonPath', pythonPath); expect(debugConfig).to.have.property('type', provider.debugType); expect(debugConfig).to.have.property('request', 'launch'); expect(debugConfig).to.have.property('program', activeFile); - expect(debugConfig).to.have.property('cwd', Uri.file(defaultWorkspace).fsPath); - expect(debugConfig).to.have.property('envFile', path.join(Uri.file(defaultWorkspace).fsPath, '.env')); + expect(debugConfig).to.have.property('cwd'); + expect(debugConfig!.cwd!.toLowerCase()).to.be.equal(filePath.toLowerCase()); + expect(debugConfig).to.have.property('envFile'); + expect(debugConfig!.envFile!.toLowerCase()).to.be.equal(path.join(filePath, '.env').toLowerCase()); expect(debugConfig).to.have.property('env'); // tslint:disable-next-line:no-any expect(Object.keys((debugConfig as any).env)).to.have.lengthOf(0); diff --git a/src/test/interpreters/display.test.ts b/src/test/interpreters/display.test.ts index 57afc8890cf0..84a7219dbc5a 100644 --- a/src/test/interpreters/display.test.ts +++ b/src/test/interpreters/display.test.ts @@ -104,7 +104,7 @@ suite('Interpreters Display', () => { statusBar.verify(s => s.text = TypeMoq.It.isValue(activeInterpreter.displayName)!, TypeMoq.Times.once()); statusBar.verify(s => s.tooltip = TypeMoq.It.isValue(expectedTooltip)!, TypeMoq.Times.once()); }); - test('If interpreter is not idenfied then tooltip should point to python Path and text containing the folder name', async () => { + test('If interpreter is not identified then tooltip should point to python Path and text containing the folder name', async () => { const resource = Uri.file('x'); const pythonPath = path.join('user', 'development', 'env', 'bin', 'python'); const workspaceFolder = Uri.file('workspace'); @@ -121,7 +121,7 @@ suite('Interpreters Display', () => { statusBar.verify(s => s.tooltip = TypeMoq.It.isValue(pythonPath), TypeMoq.Times.once()); statusBar.verify(s => s.text = TypeMoq.It.isValue(`${path.basename(pythonPath)} [Environment]`), TypeMoq.Times.once()); }); - test('If virtual environment interpreter is not idenfied then text should contain the type of virtual environment', async () => { + test('If virtual environment interpreter is not identified then text should contain the type of virtual environment', async () => { const resource = Uri.file('x'); const pythonPath = path.join('user', 'development', 'env', 'bin', 'python'); const workspaceFolder = Uri.file('workspace'); @@ -131,7 +131,7 @@ suite('Interpreters Display', () => { configurationService.setup(c => c.getSettings(TypeMoq.It.isAny())).returns(() => pythonSettings.object); pythonSettings.setup(p => p.pythonPath).returns(() => pythonPath); // tslint:disable-next-line:no-any - virtualEnvMgr.setup(v => v.detect(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve({ name: 'Mock Name' } as any)); + virtualEnvMgr.setup(v => v.detect(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve('Mock Name')); versionProvider.setup(v => v.getVersion(TypeMoq.It.isValue(pythonPath), TypeMoq.It.isAny())).returns((_path, defaultDisplayName) => Promise.resolve(defaultDisplayName)); await interpreterDisplay.refresh(resource); @@ -173,7 +173,7 @@ suite('Interpreters Display', () => { const defaultDisplayName = `${path.basename(pythonPath)} [Environment]`; versionProvider.setup(v => v.getVersion(TypeMoq.It.isValue(pythonPath), TypeMoq.It.isAny())).returns(() => Promise.resolve(defaultDisplayName)); // tslint:disable-next-line:no-any - virtualEnvMgr.setup(v => v.detect(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve({ name: 'Mock Env Name' } as any)); + virtualEnvMgr.setup(v => v.detect(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve('Mock Env Name')); const expectedText = `${defaultDisplayName} (Mock Env Name)`; await interpreterDisplay.refresh(resource); From a53d815f6531d7477481cdff45bd57fa9d3a94af Mon Sep 17 00:00:00 2001 From: MikhailArkhipov Date: Tue, 20 Feb 2018 11:56:33 -0800 Subject: [PATCH 24/32] Rename method --- src/client/interpreter/display/index.ts | 2 +- src/client/interpreter/index.ts | 2 +- .../locators/services/baseVirtualEnvService.ts | 2 +- .../locators/services/currentPathService.ts | 2 +- src/client/interpreter/virtualEnvs/index.ts | 2 +- src/client/interpreter/virtualEnvs/types.ts | 2 +- src/test/interpreters/display.test.ts | 10 +++++----- 7 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/client/interpreter/display/index.ts b/src/client/interpreter/display/index.ts index 2c73c46bb0ae..ff5abaddd331 100644 --- a/src/client/interpreter/display/index.ts +++ b/src/client/interpreter/display/index.ts @@ -82,6 +82,6 @@ export class InterpreterDisplay implements IInterpreterDisplay { this.statusBar.show(); } private async getVirtualEnvironmentName(pythonPath: string): Promise { - return this.virtualEnvMgr.detect(pythonPath); + return this.virtualEnvMgr.getEnvironmentName(pythonPath); } } diff --git a/src/client/interpreter/index.ts b/src/client/interpreter/index.ts index 9d50eeb5620e..485766343d3d 100644 --- a/src/client/interpreter/index.ts +++ b/src/client/interpreter/index.ts @@ -102,7 +102,7 @@ export class InterpreterManager implements Disposable, IInterpreterService { const pythonExecutableName = path.basename(fullyQualifiedPath); const versionInfo = await this.serviceContainer.get(IInterpreterVersionService).getVersion(fullyQualifiedPath, pythonExecutableName); const virtualEnvManager = this.serviceContainer.get(IVirtualEnvironmentManager); - const virtualEnvName = await virtualEnvManager.detect(fullyQualifiedPath); + const virtualEnvName = await virtualEnvManager.getEnvironmentName(fullyQualifiedPath); const dislayNameSuffix = virtualEnvName.length > 0 ? ` (${virtualEnvName})` : ''; const displayName = `${versionInfo}${dislayNameSuffix}`; return { diff --git a/src/client/interpreter/locators/services/baseVirtualEnvService.ts b/src/client/interpreter/locators/services/baseVirtualEnvService.ts index 85234fc2589a..fe19dd221f8d 100644 --- a/src/client/interpreter/locators/services/baseVirtualEnvService.ts +++ b/src/client/interpreter/locators/services/baseVirtualEnvService.ts @@ -67,7 +67,7 @@ export class BaseVirtualEnvService extends CacheableLocatorService { private async getVirtualEnvDetails(interpreter: string): Promise { return Promise.all([ this.versionProvider.getVersion(interpreter, path.basename(interpreter)), - this.virtualEnvMgr.detect(interpreter) + this.virtualEnvMgr.getEnvironmentName(interpreter) ]) .then(([displayName, virtualEnvName]) => { const virtualEnvSuffix = virtualEnvName.length ? virtualEnvName : this.getVirtualEnvironmentRootDirectory(interpreter); diff --git a/src/client/interpreter/locators/services/currentPathService.ts b/src/client/interpreter/locators/services/currentPathService.ts index d2c15d8dc778..8b31e1968474 100644 --- a/src/client/interpreter/locators/services/currentPathService.ts +++ b/src/client/interpreter/locators/services/currentPathService.ts @@ -37,7 +37,7 @@ export class CurrentPathService extends CacheableLocatorService { private async getInterpreterDetails(interpreter: string): Promise { return Promise.all([ this.versionProvider.getVersion(interpreter, path.basename(interpreter)), - this.virtualEnvMgr.detect(interpreter) + this.virtualEnvMgr.getEnvironmentName(interpreter) ]). then(([displayName, virtualEnvName]) => { displayName += virtualEnvName.length > 0 ? ` (${virtualEnvName})` : ''; diff --git a/src/client/interpreter/virtualEnvs/index.ts b/src/client/interpreter/virtualEnvs/index.ts index 8798de113ca4..af5545398696 100644 --- a/src/client/interpreter/virtualEnvs/index.ts +++ b/src/client/interpreter/virtualEnvs/index.ts @@ -12,7 +12,7 @@ export class VirtualEnvironmentManager implements IVirtualEnvironmentManager { constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) { this.processService = serviceContainer.get(IProcessService); } - public async detect(pythonPath: string): Promise { + public async getEnvironmentName(pythonPath: string): Promise { // https://stackoverflow.com/questions/1871549/determine-if-python-is-running-inside-virtualenv const output = await this.processService.exec(pythonPath, ['-c', 'import sys;print(hasattr(sys, "real_prefix"))']); if (output.stdout.length > 0) { diff --git a/src/client/interpreter/virtualEnvs/types.ts b/src/client/interpreter/virtualEnvs/types.ts index 3ecdc18f36e5..971772fd009d 100644 --- a/src/client/interpreter/virtualEnvs/types.ts +++ b/src/client/interpreter/virtualEnvs/types.ts @@ -3,5 +3,5 @@ export const IVirtualEnvironmentManager = Symbol('VirtualEnvironmentManager'); export interface IVirtualEnvironmentManager { - detect(pythonPath: string): Promise; + getEnvironmentName(pythonPath: string): Promise; } diff --git a/src/test/interpreters/display.test.ts b/src/test/interpreters/display.test.ts index 84a7219dbc5a..fad85341c2a4 100644 --- a/src/test/interpreters/display.test.ts +++ b/src/test/interpreters/display.test.ts @@ -113,7 +113,7 @@ suite('Interpreters Display', () => { interpreterService.setup(i => i.getActiveInterpreter(TypeMoq.It.isValue(workspaceFolder))).returns(() => Promise.resolve(undefined)); configurationService.setup(c => c.getSettings(TypeMoq.It.isAny())).returns(() => pythonSettings.object); pythonSettings.setup(p => p.pythonPath).returns(() => pythonPath); - virtualEnvMgr.setup(v => v.detect(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve('')); + virtualEnvMgr.setup(v => v.getEnvironmentName(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve('')); versionProvider.setup(v => v.getVersion(TypeMoq.It.isValue(pythonPath), TypeMoq.It.isAny())).returns((_path, defaultDisplayName) => Promise.resolve(defaultDisplayName)); await interpreterDisplay.refresh(resource); @@ -131,7 +131,7 @@ suite('Interpreters Display', () => { configurationService.setup(c => c.getSettings(TypeMoq.It.isAny())).returns(() => pythonSettings.object); pythonSettings.setup(p => p.pythonPath).returns(() => pythonPath); // tslint:disable-next-line:no-any - virtualEnvMgr.setup(v => v.detect(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve('Mock Name')); + virtualEnvMgr.setup(v => v.getEnvironmentName(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve('Mock Name')); versionProvider.setup(v => v.getVersion(TypeMoq.It.isValue(pythonPath), TypeMoq.It.isAny())).returns((_path, defaultDisplayName) => Promise.resolve(defaultDisplayName)); await interpreterDisplay.refresh(resource); @@ -152,7 +152,7 @@ suite('Interpreters Display', () => { fileSystem.setup(f => f.fileExistsAsync(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve(false)); const defaultDisplayName = `${path.basename(pythonPath)} [Environment]`; versionProvider.setup(v => v.getVersion(TypeMoq.It.isValue(pythonPath), TypeMoq.It.isAny())).returns(() => Promise.resolve(defaultDisplayName)); - virtualEnvMgr.setup(v => v.detect(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve('')); + virtualEnvMgr.setup(v => v.getEnvironmentName(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve('')); await interpreterDisplay.refresh(resource); @@ -173,7 +173,7 @@ suite('Interpreters Display', () => { const defaultDisplayName = `${path.basename(pythonPath)} [Environment]`; versionProvider.setup(v => v.getVersion(TypeMoq.It.isValue(pythonPath), TypeMoq.It.isAny())).returns(() => Promise.resolve(defaultDisplayName)); // tslint:disable-next-line:no-any - virtualEnvMgr.setup(v => v.detect(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve('Mock Env Name')); + virtualEnvMgr.setup(v => v.getEnvironmentName(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve('Mock Env Name')); const expectedText = `${defaultDisplayName} (Mock Env Name)`; await interpreterDisplay.refresh(resource); @@ -194,7 +194,7 @@ suite('Interpreters Display', () => { const displayName = 'Version from Interperter'; versionProvider.setup(v => v.getVersion(TypeMoq.It.isValue(pythonPath), TypeMoq.It.isAny())).returns(() => Promise.resolve(displayName)); // tslint:disable-next-line:no-any - virtualEnvMgr.setup(v => v.detect(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve('')); + virtualEnvMgr.setup(v => v.getEnvironmentName(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve('')); await interpreterDisplay.refresh(resource); From 7f3e4fa62d2b69e48b22fd95783a4927e595a827 Mon Sep 17 00:00:00 2001 From: MikhailArkhipov Date: Tue, 20 Feb 2018 12:17:41 -0800 Subject: [PATCH 25/32] #815 - 'F' in flake8 means warning --- package.json | 4 ++-- src/client/common/configSettings.ts | 7 +++++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/package.json b/package.json index 5bcea42a1452..d69de7260ef3 100644 --- a/package.json +++ b/package.json @@ -1297,7 +1297,7 @@ }, "python.linting.flake8CategorySeverity.F": { "type": "string", - "default": "Error", + "default": "Warning", "description": "Severity of Flake8 message type 'F'.", "enum": [ "Hint", @@ -1859,4 +1859,4 @@ "publisherDisplayName": "Microsoft", "publisherId": "998b010b-e2af-44a5-a6cd-0b5fd3b9b6f8" } -} +} \ No newline at end of file diff --git a/src/client/common/configSettings.ts b/src/client/common/configSettings.ts index 42c60d57175e..ef806ec2ee46 100644 --- a/src/client/common/configSettings.ts +++ b/src/client/common/configSettings.ts @@ -168,9 +168,12 @@ export class PythonSettings extends EventEmitter implements IPythonSettings { W: vscode.DiagnosticSeverity.Warning }, flake8CategorySeverity: { - F: vscode.DiagnosticSeverity.Error, E: vscode.DiagnosticSeverity.Error, - W: vscode.DiagnosticSeverity.Warning + W: vscode.DiagnosticSeverity.Warning, + // Per http://flake8.pycqa.org/en/latest/glossary.html#term-error-code + // 'F' does not mean 'fatal as in PyLint but rather 'pyflakes' such as + // unused imports, variables, etc. + F: vscode.DiagnosticSeverity.Warning }, mypyCategorySeverity: { error: vscode.DiagnosticSeverity.Error, From da034f40319bc8071ed01c061873c96d54072a56 Mon Sep 17 00:00:00 2001 From: MikhailArkhipov Date: Tue, 20 Feb 2018 13:43:01 -0800 Subject: [PATCH 26/32] 730 - same folder temp --- src/client/common/editor.ts | 24 ++++++++-------- src/client/formatters/baseFormatter.ts | 38 ++++++++++++++++++-------- 2 files changed, 40 insertions(+), 22 deletions(-) diff --git a/src/client/common/editor.ts b/src/client/common/editor.ts index 24be7cb209fc..7d7df1d33ab4 100644 --- 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 { return new Promise((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) { + return 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 index f678665fc02d..3322ed99c52f 100644 --- 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,26 +51,24 @@ 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); 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 @@ -77,10 +76,7 @@ export abstract class BaseFormatter { 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,24 @@ export abstract class BaseFormatter { this.outputChannel.appendLine(`\n${customError}\n${error}`); } + + private async createTempFile(document: vscode.TextDocument): Promise { + if (document.isDirty) { + return getTempFileWithDocumentContents(document); + } + return Promise.resolve(document.fileName); + } + private deleteTempFile(originalFile: string, tempFile: string): Promise { + 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; + } } From 71a508d839b734f0cab4e31d05ab9b1359b94982 Mon Sep 17 00:00:00 2001 From: MikhailArkhipov Date: Tue, 20 Feb 2018 15:17:55 -0800 Subject: [PATCH 27/32] Properly resolve ~ --- src/client/linters/pylint.ts | 14 +++++++------- src/test/linters/pylint.test.ts | 5 +++-- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/client/linters/pylint.ts b/src/client/linters/pylint.ts index 53caae8a8669..b5aaf43c3743 100644 --- 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/linters/pylint.test.ts b/src/test/linters/pylint.test.ts index 75cd78f148f3..ce08b0ac5c95 100644 --- 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,7 +101,7 @@ 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)); @@ -108,7 +109,7 @@ suite('Linting - Pylintrc search', () => { 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)); From 6d919120b75235a1ffe92847883d89127e95dda5 Mon Sep 17 00:00:00 2001 From: MikhailArkhipov Date: Tue, 20 Feb 2018 17:20:09 -0800 Subject: [PATCH 28/32] Test --- src/test/format/extension.format.test.ts | 49 ++++++++++++++++++- .../pythonFiles/formatting/formatWhenDirty.py | 3 ++ .../formatting/formatWhenDirtyResult.py | 5 ++ 3 files changed, 56 insertions(+), 1 deletion(-) create mode 100644 src/test/pythonFiles/formatting/formatWhenDirty.py create mode 100644 src/test/pythonFiles/formatting/formatWhenDirtyResult.py diff --git a/src/test/format/extension.format.test.ts b/src/test/format/extension.format.test.ts index 8a9d09dff517..6fa35be0dbe2 100644 --- 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); + 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=3\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/pythonFiles/formatting/formatWhenDirty.py b/src/test/pythonFiles/formatting/formatWhenDirty.py new file mode 100644 index 000000000000..b80488a5c757 --- /dev/null +++ b/src/test/pythonFiles/formatting/formatWhenDirty.py @@ -0,0 +1,3 @@ +x = 0 +if x > 0: + x = 1 \ No newline at end of file diff --git a/src/test/pythonFiles/formatting/formatWhenDirtyResult.py b/src/test/pythonFiles/formatting/formatWhenDirtyResult.py new file mode 100644 index 000000000000..921dd18533de --- /dev/null +++ b/src/test/pythonFiles/formatting/formatWhenDirtyResult.py @@ -0,0 +1,5 @@ + + +x = 0 +if x > 0: + x = 1 \ No newline at end of file From ff6f6d27a31a6bbbdd049bc2eb413eb1190306c1 Mon Sep 17 00:00:00 2001 From: Mikhail Arkhipov Date: Tue, 20 Feb 2018 21:16:13 -0800 Subject: [PATCH 29/32] Test --- src/client/common/editor.ts | 2 +- src/client/formatters/baseFormatter.ts | 9 +++++---- src/test/format/extension.format.test.ts | 4 ++-- src/test/pythonFiles/formatting/formatWhenDirty.py | 2 +- src/test/pythonFiles/formatting/formatWhenDirtyResult.py | 4 +--- 5 files changed, 10 insertions(+), 11 deletions(-) diff --git a/src/client/common/editor.ts b/src/client/common/editor.ts index 7d7df1d33ab4..e839a13e6a8b 100644 --- a/src/client/common/editor.ts +++ b/src/client/common/editor.ts @@ -231,7 +231,7 @@ export function getTempFileWithDocumentContents(document: TextDocument): Promise const fileName = `${document.uri.fsPath}.${md5(document.uri.fsPath)}${ext}`; fs.writeFile(fileName, document.getText(), ex => { if (ex) { - return reject(`Failed to create a temporary file, ${ex.message}`); + reject(`Failed to create a temporary file, ${ex.message}`); } resolve(fileName); }); diff --git a/src/client/formatters/baseFormatter.ts b/src/client/formatters/baseFormatter.ts index 3322ed99c52f..9e91b00a5a19 100644 --- a/src/client/formatters/baseFormatter.ts +++ b/src/client/formatters/baseFormatter.ts @@ -99,17 +99,18 @@ export abstract class BaseFormatter { } private async createTempFile(document: vscode.TextDocument): Promise { - if (document.isDirty) { - return getTempFileWithDocumentContents(document); - } - return Promise.resolve(document.fileName); + return document.isDirty + ? await getTempFileWithDocumentContents(document) + : document.fileName; } + private deleteTempFile(originalFile: string, tempFile: string): Promise { 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(); diff --git a/src/test/format/extension.format.test.ts b/src/test/format/extension.format.test.ts index 6fa35be0dbe2..bb90c3080e5c 100644 --- a/src/test/format/extension.format.test.ts +++ b/src/test/format/extension.format.test.ts @@ -116,7 +116,7 @@ suite('Formatting', () => { const textDocument = await vscode.workspace.openTextDocument(fileToFormat); const textEditor = await vscode.window.showTextDocument(textDocument); - textEditor.edit(builder => { + await textEditor.edit(builder => { // Make file dirty. Trailing blanks will be removed. builder.insert(new vscode.Position(0, 0), '\n \n'); }); @@ -125,7 +125,7 @@ suite('Formatting', () => { const configFile = path.join(dir, '.style.yapf'); try { // Create yapf configuration file - const content = '[style]\nbased_on_style = pep8\nindent_width=3\n'; + 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 }; diff --git a/src/test/pythonFiles/formatting/formatWhenDirty.py b/src/test/pythonFiles/formatting/formatWhenDirty.py index b80488a5c757..3fe1b80fde86 100644 --- a/src/test/pythonFiles/formatting/formatWhenDirty.py +++ b/src/test/pythonFiles/formatting/formatWhenDirty.py @@ -1,3 +1,3 @@ x = 0 if x > 0: - x = 1 \ No newline at end of file + x = 1 diff --git a/src/test/pythonFiles/formatting/formatWhenDirtyResult.py b/src/test/pythonFiles/formatting/formatWhenDirtyResult.py index 921dd18533de..d0ae06a2a59b 100644 --- a/src/test/pythonFiles/formatting/formatWhenDirtyResult.py +++ b/src/test/pythonFiles/formatting/formatWhenDirtyResult.py @@ -1,5 +1,3 @@ - - x = 0 if x > 0: - x = 1 \ No newline at end of file + x = 1 From a7b2854e01261fb47e56feccba47f68671e73d7b Mon Sep 17 00:00:00 2001 From: MikhailArkhipov Date: Wed, 21 Feb 2018 10:37:06 -0800 Subject: [PATCH 30/32] Fix dot spacing --- .../common/installer/pythonInstallation.ts | 6 ++- src/client/formatters/lineFormatter.ts | 42 +++++++++++----- src/client/language/tokenizer.ts | 4 +- .../format/extension.onEnterFormat.test.ts | 49 +++++++++++++------ src/test/install/pythonInstallation.test.ts | 17 ++++++- src/test/language/tokenizer.test.ts | 16 ++++-- .../formatting/fileToFormatOnEnter.py | 2 + 7 files changed, 100 insertions(+), 36 deletions(-) diff --git a/src/client/common/installer/pythonInstallation.ts b/src/client/common/installer/pythonInstallation.ts index 1463404eb0ab..13af1b958104 100644 --- a/src/client/common/installer/pythonInstallation.ts +++ b/src/client/common/installer/pythonInstallation.ts @@ -35,8 +35,10 @@ export class PythonInstaller { return true; } - await this.shell.showErrorMessage('Python is not installed. Please download and install Python before using the extension.'); - this.shell.openUrl('https://www.python.org/downloads'); + const download = 'Download'; + if (await this.shell.showErrorMessage('Python is not installed. Please download and install Python before using the extension.', download) === download) { + this.shell.openUrl('https://www.python.org/downloads'); + } return false; } } diff --git a/src/client/formatters/lineFormatter.ts b/src/client/formatters/lineFormatter.ts index 74286590dc5e..4b3bff70aa8d 100644 --- a/src/client/formatters/lineFormatter.ts +++ b/src/client/formatters/lineFormatter.ts @@ -48,7 +48,7 @@ export class LineFormatter { break; case TokenType.Identifier: - if (!prev || (!this.isOpenBraceType(prev.type) && prev.type !== TokenType.Colon)) { + if (prev && !this.isOpenBraceType(prev.type) && prev.type !== TokenType.Colon && prev.type !== TokenType.Operator) { this.builder.softAppendSpace(); } this.builder.append(this.text.substring(t.start, t.end)); @@ -81,17 +81,22 @@ export class LineFormatter { private handleOperator(index: number): void { const t = this.tokens.getItemAt(index); - if (index >= 2 && t.length === 1 && this.text.charCodeAt(t.start) === Char.Equal) { - if (this.braceCounter.isOpened(TokenType.OpenBrace)) { - // Check if this is = in function arguments. If so, do not - // add spaces around it. - const prev = this.tokens.getItemAt(index - 1); - const prevPrev = this.tokens.getItemAt(index - 2); - if (prev.type === TokenType.Identifier && - (prevPrev.type === TokenType.Comma || prevPrev.type === TokenType.OpenBrace)) { - this.builder.append('='); + if (t.length === 1) { + const opCode = this.text.charCodeAt(t.start); + switch (opCode) { + case Char.Equal: + if (index >= 2 && this.handleEqual(t, index)) { + return; + } + break; + case Char.Period: + this.builder.append('.'); + return; + case Char.At: + this.builder.append('@'); return; - } + default: + break; } } this.builder.softAppendSpace(); @@ -99,6 +104,21 @@ export class LineFormatter { this.builder.softAppendSpace(); } + private handleEqual(t: IToken, index: number): boolean { + if (this.braceCounter.isOpened(TokenType.OpenBrace)) { + // Check if this is = in function arguments. If so, do not + // add spaces around it. + const prev = this.tokens.getItemAt(index - 1); + const prevPrev = this.tokens.getItemAt(index - 2); + if (prev.type === TokenType.Identifier && + (prevPrev.type === TokenType.Comma || prevPrev.type === TokenType.OpenBrace)) { + this.builder.append('='); + return true; + } + } + return false; + } + private handleOther(t: IToken): void { if (this.isBraceType(t.type)) { this.braceCounter.countBrace(t); diff --git a/src/client/language/tokenizer.ts b/src/client/language/tokenizer.ts index 1a4d800fed0c..ecd382d96541 100644 --- a/src/client/language/tokenizer.ts +++ b/src/client/language/tokenizer.ts @@ -127,9 +127,9 @@ export class Tokenizer implements ITokenizer { case Char.Colon: this.tokens.push(new Token(TokenType.Colon, this.cs.position, 1)); break; - case Char.Period: case Char.At: - this.tokens.push(new Token(TokenType.Unknown, this.cs.position, 1)); + case Char.Period: + this.tokens.push(new Token(TokenType.Operator, this.cs.position, 1)); break; default: if (this.isPossibleNumber()) { diff --git a/src/test/format/extension.onEnterFormat.test.ts b/src/test/format/extension.onEnterFormat.test.ts index 1622975a39d2..74597ce19be7 100644 --- a/src/test/format/extension.onEnterFormat.test.ts +++ b/src/test/format/extension.onEnterFormat.test.ts @@ -12,47 +12,64 @@ const unformattedFile = path.join(formatFilesPath, 'fileToFormatOnEnter.py'); suite('Formatting - OnEnter provider', () => { let document: vscode.TextDocument; + let editor: vscode.TextEditor; suiteSetup(initialize); setup(async () => { document = await vscode.workspace.openTextDocument(unformattedFile); - await vscode.window.showTextDocument(document); + editor = await vscode.window.showTextDocument(document); }); suiteTeardown(closeActiveWindows); teardown(closeActiveWindows); - test('Regular string', async () => { - const edits = await formatAtPosition(1, 0); - assert.notEqual(edits!.length, 0, 'Line was not formatted'); + test('Simple statement', async () => { + const text = await formatAtPosition(1, 0); + assert.equal(text, 'x = 1', 'Line was not formatted'); }); test('No formatting inside strings', async () => { - const edits = await formatAtPosition(2, 0); - assert.equal(edits!.length, 0, 'Text inside string was formatted'); + let text = await formatAtPosition(2, 0); + assert.equal(text, '"""x=1', 'Text inside string was formatted'); + text = await formatAtPosition(3, 0); + assert.equal(text, '"""', 'Text inside string was formatted'); }); test('Whitespace before comment', async () => { - const edits = await formatAtPosition(4, 0); - assert.equal(edits!.length, 0, 'Whitespace before comment was formatted'); + const text = await formatAtPosition(4, 0); + assert.equal(text, ' # comment', 'Whitespace before comment was not preserved'); }); test('No formatting of comment', async () => { - const edits = await formatAtPosition(5, 0); - assert.equal(edits!.length, 0, 'Text inside comment was formatted'); + const text = await formatAtPosition(5, 0); + assert.equal(text, '# x=1', 'Text inside comment was formatted'); }); test('Formatting line ending in comment', async () => { - const edits = await formatAtPosition(6, 0); - assert.notEqual(edits!.length, 0, 'Line ending in comment was not formatted'); + const text = await formatAtPosition(6, 0); + assert.equal(text, 'x + 1 # ', 'Line ending in comment was not formatted'); + }); + + test('Formatting line with @', async () => { + const text = await formatAtPosition(7, 0); + assert.equal(text, '@x', 'Line with @ was reformatted'); + }); + + test('Formatting line with @', async () => { + const text = await formatAtPosition(8, 0); + assert.equal(text, 'x.y', 'Line ending with period was reformatted'); }); test('Formatting line ending in string', async () => { - const edits = await formatAtPosition(7, 0); - assert.notEqual(edits!.length, 0, 'Line ending in multilint string was not formatted'); + const text = await formatAtPosition(9, 0); + assert.equal(text, 'x + """', 'Line ending in multiline string was not formatted'); }); - async function formatAtPosition(line: number, character: number): Promise { - return await vscode.commands.executeCommand('vscode.executeFormatOnTypeProvider', + async function formatAtPosition(line: number, character: number): Promise { + const edits = await vscode.commands.executeCommand('vscode.executeFormatOnTypeProvider', document.uri, new vscode.Position(line, character), '\n', { insertSpaces: true, tabSize: 2 }); + if (edits) { + await editor.edit(builder => edits.forEach(e => builder.replace(e.range, e.newText))); + } + return document.lineAt(line - 1).text; } }); diff --git a/src/test/install/pythonInstallation.test.ts b/src/test/install/pythonInstallation.test.ts index 4df7dfdd4fb2..23e5429c1bb9 100644 --- a/src/test/install/pythonInstallation.test.ts +++ b/src/test/install/pythonInstallation.test.ts @@ -81,7 +81,11 @@ suite('Installation', () => { let openUrlCalled = false; let url; - c.appShell.setup(x => x.showErrorMessage(TypeMoq.It.isAnyString())).callback(() => showErrorMessageCalled = true); + const download = 'Download'; + c.appShell + .setup(x => x.showErrorMessage(TypeMoq.It.isAnyString(), download)) + .callback(() => showErrorMessageCalled = true) + .returns(() => Promise.resolve(download)); c.appShell.setup(x => x.openUrl(TypeMoq.It.isAnyString())).callback((s: string) => { openUrlCalled = true; url = s; @@ -93,6 +97,17 @@ suite('Installation', () => { assert.equal(showErrorMessageCalled, true, 'Error message not shown'); assert.equal(openUrlCalled, true, 'Python download page not opened'); assert.equal(url, 'https://www.python.org/downloads', 'Python download page is incorrect'); + + showErrorMessageCalled = false; + openUrlCalled = false; + c.appShell + .setup(x => x.showErrorMessage(TypeMoq.It.isAnyString(), download)) + .callback(() => showErrorMessageCalled = true) + .returns(() => Promise.resolve('')); + + await c.pythonInstaller.checkPythonInstallation(c.settings.object); + assert.equal(showErrorMessageCalled, true, 'Error message not shown'); + assert.equal(openUrlCalled, false, 'Python download page was opened'); }); test('Mac: Default Python warning', async () => { diff --git a/src/test/language/tokenizer.test.ts b/src/test/language/tokenizer.test.ts index e11df6a147b0..e2a5b3f6defb 100644 --- a/src/test/language/tokenizer.test.ts +++ b/src/test/language/tokenizer.test.ts @@ -91,15 +91,23 @@ suite('Language.Tokenizer', () => { assert.equal(tokens.getItemAt(i).type, TokenType.Comment); } }); - test('Period/At to unknown token', async () => { + test('Period to operator token', async () => { const t = new Tokenizer(); - const tokens = t.tokenize('.@x'); + const tokens = t.tokenize('x.y'); assert.equal(tokens.count, 3); - assert.equal(tokens.getItemAt(0).type, TokenType.Unknown); - assert.equal(tokens.getItemAt(1).type, TokenType.Unknown); + assert.equal(tokens.getItemAt(0).type, TokenType.Identifier); + assert.equal(tokens.getItemAt(1).type, TokenType.Operator); assert.equal(tokens.getItemAt(2).type, TokenType.Identifier); }); + test('@ to operator token', async () => { + const t = new Tokenizer(); + const tokens = t.tokenize('@x'); + assert.equal(tokens.count, 2); + + assert.equal(tokens.getItemAt(0).type, TokenType.Operator); + assert.equal(tokens.getItemAt(1).type, TokenType.Identifier); + }); test('Unknown token', async () => { const t = new Tokenizer(); const tokens = t.tokenize('~$'); diff --git a/src/test/pythonFiles/formatting/fileToFormatOnEnter.py b/src/test/pythonFiles/formatting/fileToFormatOnEnter.py index 6f680ad49b0d..bbd025363098 100644 --- a/src/test/pythonFiles/formatting/fileToFormatOnEnter.py +++ b/src/test/pythonFiles/formatting/fileToFormatOnEnter.py @@ -4,4 +4,6 @@ # comment # x=1 x+1 # +@x +x.y x+""" From c7c07deee789d2630007ff9ae59b596f0d6f383e Mon Sep 17 00:00:00 2001 From: MikhailArkhipov Date: Wed, 21 Feb 2018 12:38:58 -0800 Subject: [PATCH 31/32] Remove banner --- src/client/extension.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/client/extension.ts b/src/client/extension.ts index 4c3cb509d6de..4788030e98de 100644 --- a/src/client/extension.ts +++ b/src/client/extension.ts @@ -8,7 +8,6 @@ if ((Reflect as any).metadata === undefined) { import { Container } from 'inversify'; import * as vscode from 'vscode'; import { Disposable, Memento, OutputChannel, window } from 'vscode'; -import { BannerService } from './banner'; import { PythonSettings } from './common/configSettings'; import * as settings from './common/configSettings'; import { STANDARD_OUTPUT_CHANNEL } from './common/constants'; @@ -183,9 +182,6 @@ export async function activate(context: vscode.ExtensionContext) { }); activationDeferred.resolve(); - // tslint:disable-next-line:no-unused-expression - new BannerService(persistentStateFactory); - const deprecationMgr = new FeatureDeprecationManager(persistentStateFactory, !!jupyterExtension); deprecationMgr.initialize(); context.subscriptions.push(new FeatureDeprecationManager(persistentStateFactory, !!jupyterExtension)); From e9eec39e2fe86bf68122995f9ae6e3ecf93b50cc Mon Sep 17 00:00:00 2001 From: MikhailArkhipov Date: Wed, 21 Feb 2018 13:14:06 -0800 Subject: [PATCH 32/32] Delete banner code --- src/client/banner.ts | 34 ---------------------------------- 1 file changed, 34 deletions(-) delete mode 100644 src/client/banner.ts diff --git a/src/client/banner.ts b/src/client/banner.ts deleted file mode 100644 index 67fc890ff951..000000000000 --- a/src/client/banner.ts +++ /dev/null @@ -1,34 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -import { window } from 'vscode'; -import { launch } from './common/net/browser'; -import { IPersistentState, IPersistentStateFactory } from './common/types'; - -const BANNER_URL = 'https://aka.ms/pvsc-at-msft'; - -export class BannerService { - private shouldShowBanner: IPersistentState; - constructor(persistentStateFactory: IPersistentStateFactory) { - this.shouldShowBanner = persistentStateFactory.createGlobalPersistentState('SHOW_NEW_PUBLISHER_BANNER', true); - this.showBanner(); - } - private showBanner() { - if (!this.shouldShowBanner.value) { - return; - } - this.shouldShowBanner.updateValue(false) - .catch(ex => console.error('Python Extension: Failed to update banner value', ex)); - - const message = 'The Python extension is now published by Microsoft!'; - const yesButton = 'Read more'; - window.showInformationMessage(message, yesButton).then((value) => { - if (value === yesButton) { - this.displayBanner(); - } - }); - } - private displayBanner() { - launch(BANNER_URL); - } -}