From 2e5aaf378291b48a6406058b175934ff58b92ab9 Mon Sep 17 00:00:00 2001 From: Derek Keeler Date: Tue, 16 Oct 2018 17:45:09 -0700 Subject: [PATCH 01/16] Pylint no longer enabled by default. Fix for #974 - Pylint is easily enabled for workspaces that have it available --- news/1 Enhancements/974.md | 1 + package.json | 4 +- src/client/linters/baseLinter.ts | 4 - src/client/linters/linterCommands.ts | 4 +- src/client/linters/linterManager.ts | 128 ++++++- src/client/linters/lintingEngine.ts | 16 +- src/client/linters/types.ts | 8 +- src/client/providers/linterProvider.ts | 28 +- .../common/configSettings.multiroot.test.ts | 338 +++++++++--------- src/test/linters/lint.commands.test.ts | 8 +- src/test/linters/lint.manager.test.ts | 12 +- src/test/linters/lint.test.ts | 16 +- src/test/linters/lintengine.test.ts | 4 +- 13 files changed, 341 insertions(+), 230 deletions(-) create mode 100644 news/1 Enhancements/974.md diff --git a/news/1 Enhancements/974.md b/news/1 Enhancements/974.md new file mode 100644 index 000000000000..cb43b6ceedfb --- /dev/null +++ b/news/1 Enhancements/974.md @@ -0,0 +1 @@ +Pylint is no longer enabled by default. Users that have not configured pylint but who have installed it in their workspace will be asked if they'd like to enable it. diff --git a/package.json b/package.json index 1b8d5f262ba9..6501aae56d20 100644 --- a/package.json +++ b/package.json @@ -1257,7 +1257,7 @@ }, "python.linting.pylintEnabled": { "type": "boolean", - "default": true, + "default": false, "description": "Whether to lint Python files using pylint.", "scope": "resource" }, @@ -1690,4 +1690,4 @@ "publisherDisplayName": "Microsoft", "publisherId": "998b010b-e2af-44a5-a6cd-0b5fd3b9b6f8" } -} +} \ No newline at end of file diff --git a/src/client/linters/baseLinter.ts b/src/client/linters/baseLinter.ts index 2f31cf970b77..05289316c1f2 100644 --- a/src/client/linters/baseLinter.ts +++ b/src/client/linters/baseLinter.ts @@ -86,10 +86,6 @@ export abstract class BaseLinter implements ILinter { return this._info; } - public isLinterExecutableSpecified(resource: vscode.Uri) { - const executablePath = this.info.pathName(resource); - return path.basename(executablePath).length > 0 && path.basename(executablePath) !== executablePath; - } public async lint(document: vscode.TextDocument, cancellation: vscode.CancellationToken): Promise { this._pythonSettings = this.configService.getSettings(document.uri); return this.runLinter(document, cancellation); diff --git a/src/client/linters/linterCommands.ts b/src/client/linters/linterCommands.ts index 9ab7a673467a..c5e12e4f3ba0 100644 --- a/src/client/linters/linterCommands.ts +++ b/src/client/linters/linterCommands.ts @@ -28,7 +28,7 @@ export class LinterCommands implements vscode.Disposable { public async setLinterAsync(): Promise { const linters = this.linterManager.getAllLinterInfos(); const suggestions = linters.map(x => x.id).sort(); - const activeLinters = this.linterManager.getActiveLinters(this.settingsUri); + const activeLinters = await this.linterManager.getActiveLinters(false, this.settingsUri); let current: string; switch (activeLinters.length) { @@ -64,7 +64,7 @@ export class LinterCommands implements vscode.Disposable { public async enableLintingAsync(): Promise { const options = ['on', 'off']; - const current = this.linterManager.isLintingEnabled(this.settingsUri) ? options[0] : options[1]; + const current = await this.linterManager.isLintingEnabled(false, this.settingsUri) ? options[0] : options[1]; const quickPickOptions: vscode.QuickPickOptions = { matchOnDetail: true, diff --git a/src/client/linters/linterManager.ts b/src/client/linters/linterManager.ts index c07d0b48e037..24e1d01ff44d 100644 --- a/src/client/linters/linterManager.ts +++ b/src/client/linters/linterManager.ts @@ -1,9 +1,20 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +'use strict'; + import { inject, injectable } from 'inversify'; -import { CancellationToken, OutputChannel, TextDocument, Uri } from 'vscode'; -import { IConfigurationService, ILogger, Product } from '../common/types'; +import { + CancellationToken, MessageItem, OutputChannel, + TextDocument, Uri +} from 'vscode'; +import { IApplicationShell, IWorkspaceService } from '../common/application/types'; +import { STANDARD_OUTPUT_CHANNEL } from '../common/constants'; +import { LinterInstaller } from '../common/installer/productInstaller'; +import { + IConfigurationService, ILogger, + IOutputChannel, Product +} from '../common/types'; import { IServiceContainer } from '../ioc/types'; import { Bandit } from './bandit'; import { Flake8 } from './flake8'; @@ -31,8 +42,9 @@ export class LinterManager implements ILinterManager { private lintingEnabledSettingName = 'enabled'; private linters: ILinterInfo[]; private configService: IConfigurationService; + private checkedForInstalledLinters: boolean = false; - constructor(@inject(IServiceContainer) serviceContainer: IServiceContainer) { + constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) { this.configService = serviceContainer.get(IConfigurationService); this.linters = [ new LinterInfo(Product.bandit, 'bandit', this.configService), @@ -58,26 +70,116 @@ export class LinterManager implements ILinterManager { throw new Error('Invalid linter'); } - public isLintingEnabled(resource?: Uri): boolean { + public async isLintingEnabled(checkAvailable: boolean, resource?: Uri): Promise { const settings = this.configService.getSettings(resource); - return (settings.linting[this.lintingEnabledSettingName] as boolean) && this.getActiveLinters(resource).length > 0; + const activeLintersPresent = await this.getActiveLinters(checkAvailable, resource); + return (settings.linting[this.lintingEnabledSettingName] as boolean) && activeLintersPresent.length > 0; } public async enableLintingAsync(enable: boolean, resource?: Uri): Promise { await this.configService.updateSetting(`linting.${this.lintingEnabledSettingName}`, enable, resource); + } + + /// Check if it is possible to enable an otherwise-unconfigured linter in the current + /// workspace, and if so ask the user if they want that linter configured explicitly. + /// Return true if a configuration change was made. + public async notifyUserOfInstalledLinters(linterInfo: ILinterInfo, resource?: Uri): Promise { + // if we've already checked during this session, don't bother again + if (!this.checkedForInstalledLinters) { + this.checkedForInstalledLinters = true; + } else { + return false; + } + + // If linting is disabled, we are finished. + if (!await this.isLintingEnabled(false, resource)) { + return false; + } + + // Has the linter in question has been configured explicitly? If so, no need to continue. + if (!this.isLinterUnset(linterInfo, resource)) { + return false; + } - // If nothing is enabled, fix it up to PyLint (default). - if (enable && this.getActiveLinters(resource).length === 0) { - await this.setActiveLintersAsync([Product.pylint], resource); + // Is the linter available in the current workspace? + if (await this.isLinterAvailable(linterInfo.product, resource)) { + // ...ask the user if they would like to enable it. + return this.notifyUserAndConfigureLinter(linterInfo); + } else { + return false; } + } - public getActiveLinters(resource?: Uri): ILinterInfo[] { - return this.linters.filter(x => x.isEnabled(resource)); + /// Raise a dialog asking the user if they would like to explicitly configure a linter or not. + /// Return true if a config change was made. + public async notifyUserAndConfigureLinter(linterInfo: ILinterInfo): Promise { + const appShell = this.serviceContainer.get(IApplicationShell); + + class ConfigureLinterMessage implements MessageItem { + public enabled: boolean; + public title: string; + constructor() { + this.enabled = true; + this.title = ''; + } + } + + const optButtons: ConfigureLinterMessage[] = [ + { + title: `Enable ${linterInfo.id}`, + enabled: true + }, + { + title: `Disable ${linterInfo.id}`, + enabled: false + } + ]; + const pick = await appShell.showInformationMessage(`Linter ${linterInfo.id} is available but not enabled.`, ...optButtons); + if (pick) { + await linterInfo.enableAsync(pick.enabled); + return true; + } + + return false; + } + + /// Check if the linter itself is available in the workspace's Python environment or not. + /// Return true if the linter is present. + public async isLinterAvailable(linterProduct: Product, resource?: Uri): Promise { + const outputChannel = this.serviceContainer.get(IOutputChannel, STANDARD_OUTPUT_CHANNEL); + const linterInstaller = new LinterInstaller(this.serviceContainer, outputChannel); + + return linterInstaller.isInstalled(linterProduct, resource); + } + + /// Check if the given linter has been configured by the user in this workspace or not. + /// Return true if no explicit setting for the linter has been made. + public isLinterUnset(linterInfo: ILinterInfo, resource?: Uri) { + const workspaceConfig = this.serviceContainer.get(IWorkspaceService); + const ws = workspaceConfig.getConfiguration('python.linting', resource); + const pe = ws!.inspect(linterInfo.enabledSettingName); + return (pe!.globalValue === undefined && pe!.workspaceValue === undefined && pe!.workspaceFolderValue === undefined); + } + + public async getActiveLinters(checkAvailable: boolean, resource?: Uri): Promise { + let activeLinters = this.linters.filter(x => x.isEnabled(resource)); + + if (checkAvailable) { + // only ask the user if they'd like to enable pylint when it is available... others may follow. + const pylintInfo = this.linters.find((linter: ILinterInfo) => linter.id === 'pylint'); + if (pylintInfo) { + const change = await this.notifyUserOfInstalledLinters(pylintInfo, resource); + if (change) { + activeLinters = this.linters.filter(x => x.isEnabled(resource)); + } + } + } + return activeLinters; } public async setActiveLintersAsync(products: Product[], resource?: Uri): Promise { - const active = this.getActiveLinters(resource); + const active = await this.getActiveLinters(false, resource); for (const x of active) { await x.enableAsync(false, resource); } @@ -90,8 +192,8 @@ export class LinterManager implements ILinterManager { } } - public createLinter(product: Product, outputChannel: OutputChannel, serviceContainer: IServiceContainer, resource?: Uri): ILinter { - if (!this.isLintingEnabled(resource)) { + public async createLinter(product: Product, outputChannel: OutputChannel, serviceContainer: IServiceContainer, resource?: Uri): Promise { + if (!await this.isLintingEnabled(false, resource)) { return new DisabledLinter(this.configService); } const error = 'Linter manager: Unknown linter'; diff --git a/src/client/linters/lintingEngine.ts b/src/client/linters/lintingEngine.ts index 7f318f7c3b7c..bca846dc4ae5 100644 --- a/src/client/linters/lintingEngine.ts +++ b/src/client/linters/lintingEngine.ts @@ -1,6 +1,8 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +'use strict'; + import { inject, injectable } from 'inversify'; import { Minimatch } from 'minimatch'; import * as path from 'path'; @@ -93,10 +95,16 @@ export class LintingEngine implements ILintingEngine { this.pendingLintings.set(document.uri.fsPath, cancelToken); - const promises: Promise[] = this.linterManager.getActiveLinters(document.uri) - .map(info => { + const activeLinters = await this.linterManager.getActiveLinters(true, document.uri); + const promises: Promise[] = activeLinters + .map(async (info: ILinterInfo) => { const stopWatch = new StopWatch(); - const linter = this.linterManager.createLinter(info.product, this.outputChannel, this.serviceContainer, document.uri); + const linter = await this.linterManager.createLinter( + info.product, + this.outputChannel, + this.serviceContainer, + document.uri + ); const promise = linter.lint(document, cancelToken.token); this.sendLinterRunTelemetry(info, document.uri, promise, stopWatch, trigger); return promise; @@ -175,7 +183,7 @@ export class LintingEngine implements ILintingEngine { } private async shouldLintDocument(document: vscode.TextDocument): Promise { - if (!this.linterManager.isLintingEnabled(document.uri)) { + if (!await this.linterManager.isLintingEnabled(true, document.uri)) { this.diagnosticCollection.set(document.uri, []); return false; } diff --git a/src/client/linters/types.ts b/src/client/linters/types.ts index 32db0d65282b..cd09ac43f865 100644 --- a/src/client/linters/types.ts +++ b/src/client/linters/types.ts @@ -19,7 +19,7 @@ export interface ILinterInfo { readonly argsSettingName: string; readonly enabledSettingName: string; readonly configFileNames: string[]; - enableAsync(flag: boolean, resource?: vscode.Uri): Promise; + enableAsync(enabled: boolean, resource?: vscode.Uri): Promise; isEnabled(resource?: vscode.Uri): boolean; pathName(resource?: vscode.Uri): string; linterArgs(resource?: vscode.Uri): string[]; @@ -35,11 +35,11 @@ export const ILinterManager = Symbol('ILinterManager'); export interface ILinterManager { getAllLinterInfos(): ILinterInfo[]; getLinterInfo(product: Product): ILinterInfo; - getActiveLinters(resource?: vscode.Uri): ILinterInfo[]; - isLintingEnabled(resource?: vscode.Uri): boolean; + getActiveLinters(checkAvailable: boolean, resource?: vscode.Uri): Promise; + isLintingEnabled(checkAvailable: boolean, resource?: vscode.Uri): Promise; enableLintingAsync(enable: boolean, resource?: vscode.Uri): Promise; setActiveLintersAsync(products: Product[], resource?: vscode.Uri): Promise; - createLinter(product: Product, outputChannel: vscode.OutputChannel, serviceContainer: IServiceContainer, resource?: vscode.Uri): ILinter; + createLinter(product: Product, outputChannel: vscode.OutputChannel, serviceContainer: IServiceContainer, resource?: vscode.Uri): Promise; } export interface ILintMessage { diff --git a/src/client/providers/linterProvider.ts b/src/client/providers/linterProvider.ts index fb66aab3971b..274df08822a5 100644 --- a/src/client/providers/linterProvider.ts +++ b/src/client/providers/linterProvider.ts @@ -1,9 +1,13 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +'use strict'; + import * as path from 'path'; -import * as vscode from 'vscode'; -import { ConfigurationTarget, Uri, workspace } from 'vscode'; +import { + ConfigurationTarget, Disposable, ExtensionContext, + TextDocument, Uri, workspace +} from 'vscode'; import { IDocumentManager } from '../common/application/types'; import { ConfigSettingMonitor } from '../common/configSettingMonitor'; import { isTestExecution } from '../common/constants'; @@ -13,9 +17,9 @@ import { IInterpreterService } from '../interpreter/contracts'; import { IServiceContainer } from '../ioc/types'; import { ILinterManager, ILintingEngine } from '../linters/types'; -export class LinterProvider implements vscode.Disposable { - private context: vscode.ExtensionContext; - private disposables: vscode.Disposable[]; +export class LinterProvider implements Disposable { + private context: ExtensionContext; + private disposables: Disposable[]; private configMonitor: ConfigSettingMonitor; private interpreterService: IInterpreterService; private documents: IDocumentManager; @@ -24,7 +28,7 @@ export class LinterProvider implements vscode.Disposable { private engine: ILintingEngine; private fs: IFileSystem; - public constructor(context: vscode.ExtensionContext, serviceContainer: IServiceContainer) { + public constructor(context: ExtensionContext, serviceContainer: IServiceContainer) { this.context = context; this.disposables = []; @@ -39,7 +43,7 @@ export class LinterProvider implements vscode.Disposable { this.documents.onDidOpenTextDocument(e => this.onDocumentOpened(e), this.context.subscriptions); this.documents.onDidCloseTextDocument(e => this.onDocumentClosed(e), this.context.subscriptions); - this.documents.onDidSaveTextDocument((e) => this.onDocumentSaved(e), this.context.subscriptions); + this.documents.onDidSaveTextDocument(async (e) => this.onDocumentSaved(e), this.context.subscriptions); this.configMonitor = new ConfigSettingMonitor('linting'); this.configMonitor.on('change', this.lintSettingsChangedHandler.bind(this)); @@ -56,7 +60,7 @@ export class LinterProvider implements vscode.Disposable { this.configMonitor.dispose(); } - private isDocumentOpen(uri: vscode.Uri): boolean { + private isDocumentOpen(uri: Uri): boolean { return this.documents.textDocuments.some(document => this.fs.arePathsSame(document.uri.fsPath, uri.fsPath)); } @@ -74,18 +78,18 @@ export class LinterProvider implements vscode.Disposable { }); } - private onDocumentOpened(document: vscode.TextDocument): void { + private onDocumentOpened(document: TextDocument): void { this.engine.lintDocument(document, 'auto').ignoreErrors(); } - private onDocumentSaved(document: vscode.TextDocument): void { + private async onDocumentSaved(document: TextDocument): Promise { const settings = this.configuration.getSettings(document.uri); if (document.languageId === 'python' && settings.linting.enabled && settings.linting.lintOnSave) { this.engine.lintDocument(document, 'save').ignoreErrors(); return; } - const linters = this.linterManager.getActiveLinters(document.uri); + const linters = await this.linterManager.getActiveLinters(true, document.uri); const fileName = path.basename(document.uri.fsPath).toLowerCase(); const watchers = linters.filter((info) => info.configFileNames.indexOf(fileName) >= 0); if (watchers.length > 0) { @@ -93,7 +97,7 @@ export class LinterProvider implements vscode.Disposable { } } - private onDocumentClosed(document: vscode.TextDocument) { + private onDocumentClosed(document: TextDocument) { if (!document || !document.fileName || !document.uri) { return; } diff --git a/src/test/common/configSettings.multiroot.test.ts b/src/test/common/configSettings.multiroot.test.ts index 1f3190995858..00b4e2d2d702 100644 --- a/src/test/common/configSettings.multiroot.test.ts +++ b/src/test/common/configSettings.multiroot.test.ts @@ -7,178 +7,178 @@ import { closeActiveWindows, initialize, initializeTest, IS_MULTI_ROOT_TEST } fr const multirootPath = path.join(__dirname, '..', '..', '..', 'src', 'testMultiRootWkspc'); -// tslint:disable-next-line:max-func-body-length -suite('Multiroot Config Settings', () => { - suiteSetup(async function () { - if (!IS_MULTI_ROOT_TEST) { - // tslint:disable-next-line:no-invalid-this - this.skip(); +if (IS_MULTI_ROOT_TEST) { + + // tslint:disable-next-line:max-func-body-length + suite('Multiroot Config Settings', () => { + suiteSetup(async () => { + await clearPythonPathInWorkspaceFolder(Uri.file(path.join(multirootPath, 'workspace1'))); + await initialize(); + }); + setup(initializeTest); + suiteTeardown(closeActiveWindows); + teardown(async () => { + await closeActiveWindows(); + await clearPythonPathInWorkspaceFolder(Uri.file(path.join(multirootPath, 'workspace1'))); + await initializeTest(); + }); + + async function enableDisableLinterSetting(resource: Uri, configTarget: ConfigurationTarget, setting: string, enabled: boolean | undefined): Promise { + const settings = workspace.getConfiguration('python.linting', resource); + const cfgValue = settings.inspect(setting); + if (configTarget === ConfigurationTarget.Workspace && cfgValue && cfgValue.workspaceValue === enabled) { + return; + } + if (configTarget === ConfigurationTarget.WorkspaceFolder && cfgValue && cfgValue.workspaceFolderValue === enabled) { + return; + } + await settings.update(setting, enabled, configTarget); + PythonSettings.dispose(); } - await clearPythonPathInWorkspaceFolder(Uri.file(path.join(multirootPath, 'workspace1'))); - await initialize(); - }); - setup(initializeTest); - suiteTeardown(closeActiveWindows); - teardown(async () => { - await closeActiveWindows(); - await clearPythonPathInWorkspaceFolder(Uri.file(path.join(multirootPath, 'workspace1'))); - await initializeTest(); - }); - async function enableDisableLinterSetting(resource: Uri, configTarget: ConfigurationTarget, setting: string, enabled: boolean | undefined): Promise { - const settings = workspace.getConfiguration('python.linting', resource); - const cfgValue = settings.inspect(setting); - if (configTarget === ConfigurationTarget.Workspace && cfgValue && cfgValue.workspaceValue === enabled) { - return; - } - if (configTarget === ConfigurationTarget.WorkspaceFolder && cfgValue && cfgValue.workspaceFolderValue === enabled) { - return; - } - await settings.update(setting, enabled, configTarget); - PythonSettings.dispose(); - } - - test('Workspace folder should inherit Python Path from workspace root', async () => { - const workspaceUri = Uri.file(path.join(multirootPath, 'workspace1')); - let settings = workspace.getConfiguration('python', workspaceUri); - const pythonPath = `x${new Date().getTime()}`; - await settings.update('pythonPath', pythonPath, ConfigurationTarget.Workspace); - const value = settings.inspect('pythonPath'); - if (value && typeof value.workspaceFolderValue === 'string') { + test('Workspace folder should inherit Python Path from workspace root', async () => { + const workspaceUri = Uri.file(path.join(multirootPath, 'workspace1')); + let settings = workspace.getConfiguration('python', workspaceUri); + const pythonPath = `x${new Date().getTime()}`; + await settings.update('pythonPath', pythonPath, ConfigurationTarget.Workspace); + const value = settings.inspect('pythonPath'); + if (value && typeof value.workspaceFolderValue === 'string') { + await settings.update('pythonPath', undefined, ConfigurationTarget.WorkspaceFolder); + } + settings = workspace.getConfiguration('python', workspaceUri); + PythonSettings.dispose(); + const cfgSetting = PythonSettings.getInstance(workspaceUri); + assert.equal(cfgSetting.pythonPath, pythonPath, 'Python Path not inherited from workspace'); + }); + + test('Workspace folder should not inherit Python Path from workspace root', async () => { + const workspaceUri = Uri.file(path.join(multirootPath, 'workspace1')); + const settings = workspace.getConfiguration('python', workspaceUri); + const pythonPath = `x${new Date().getTime()}`; + await settings.update('pythonPath', pythonPath, ConfigurationTarget.Workspace); + const privatePythonPath = `x${new Date().getTime()}`; + await settings.update('pythonPath', privatePythonPath, ConfigurationTarget.WorkspaceFolder); + + const cfgSetting = PythonSettings.getInstance(workspaceUri); + assert.equal(cfgSetting.pythonPath, privatePythonPath, 'Python Path for workspace folder is incorrect'); + }); + + test('Workspace folder should inherit Python Path from workspace root when opening a document', async () => { + const workspaceUri = Uri.file(path.join(multirootPath, 'workspace1')); + const fileToOpen = path.join(multirootPath, 'workspace1', 'file.py'); + + const settings = workspace.getConfiguration('python', workspaceUri); + const pythonPath = `x${new Date().getTime()}`; + await settings.update('pythonPath', pythonPath, ConfigurationTarget.Workspace); + // Update workspace folder to something else so it gets refreshed. + await settings.update('pythonPath', `x${new Date().getTime()}`, ConfigurationTarget.WorkspaceFolder); await settings.update('pythonPath', undefined, ConfigurationTarget.WorkspaceFolder); - } - settings = workspace.getConfiguration('python', workspaceUri); - PythonSettings.dispose(); - const cfgSetting = PythonSettings.getInstance(workspaceUri); - assert.equal(cfgSetting.pythonPath, pythonPath, 'Python Path not inherited from workspace'); - }); - - test('Workspace folder should not inherit Python Path from workspace root', async () => { - const workspaceUri = Uri.file(path.join(multirootPath, 'workspace1')); - const settings = workspace.getConfiguration('python', workspaceUri); - const pythonPath = `x${new Date().getTime()}`; - await settings.update('pythonPath', pythonPath, ConfigurationTarget.Workspace); - const privatePythonPath = `x${new Date().getTime()}`; - await settings.update('pythonPath', privatePythonPath, ConfigurationTarget.WorkspaceFolder); - - const cfgSetting = PythonSettings.getInstance(workspaceUri); - assert.equal(cfgSetting.pythonPath, privatePythonPath, 'Python Path for workspace folder is incorrect'); - }); - - test('Workspace folder should inherit Python Path from workspace root when opening a document', async () => { - const workspaceUri = Uri.file(path.join(multirootPath, 'workspace1')); - const fileToOpen = path.join(multirootPath, 'workspace1', 'file.py'); - - const settings = workspace.getConfiguration('python', workspaceUri); - const pythonPath = `x${new Date().getTime()}`; - await settings.update('pythonPath', pythonPath, ConfigurationTarget.Workspace); - // Update workspace folder to something else so it gets refreshed. - await settings.update('pythonPath', `x${new Date().getTime()}`, ConfigurationTarget.WorkspaceFolder); - await settings.update('pythonPath', undefined, ConfigurationTarget.WorkspaceFolder); - - const document = await workspace.openTextDocument(fileToOpen); - const cfg = PythonSettings.getInstance(document.uri); - assert.equal(cfg.pythonPath, pythonPath, 'Python Path not inherited from workspace'); - }); - - test('Workspace folder should not inherit Python Path from workspace root when opening a document', async () => { - const workspaceUri = Uri.file(path.join(multirootPath, 'workspace1')); - const fileToOpen = path.join(multirootPath, 'workspace1', 'file.py'); - - const settings = workspace.getConfiguration('python', workspaceUri); - const pythonPath = `x${new Date().getTime()}`; - await settings.update('pythonPath', pythonPath, ConfigurationTarget.Workspace); - const privatePythonPath = `x${new Date().getTime()}`; - await settings.update('pythonPath', privatePythonPath, ConfigurationTarget.WorkspaceFolder); - const document = await workspace.openTextDocument(fileToOpen); - const cfg = PythonSettings.getInstance(document.uri); - assert.equal(cfg.pythonPath, privatePythonPath, 'Python Path for workspace folder is incorrect'); + const document = await workspace.openTextDocument(fileToOpen); + const cfg = PythonSettings.getInstance(document.uri); + assert.equal(cfg.pythonPath, pythonPath, 'Python Path not inherited from workspace'); + }); + + test('Workspace folder should not inherit Python Path from workspace root when opening a document', async () => { + const workspaceUri = Uri.file(path.join(multirootPath, 'workspace1')); + const fileToOpen = path.join(multirootPath, 'workspace1', 'file.py'); + + const settings = workspace.getConfiguration('python', workspaceUri); + const pythonPath = `x${new Date().getTime()}`; + await settings.update('pythonPath', pythonPath, ConfigurationTarget.Workspace); + const privatePythonPath = `x${new Date().getTime()}`; + await settings.update('pythonPath', privatePythonPath, ConfigurationTarget.WorkspaceFolder); + + const document = await workspace.openTextDocument(fileToOpen); + const cfg = PythonSettings.getInstance(document.uri); + assert.equal(cfg.pythonPath, privatePythonPath, 'Python Path for workspace folder is incorrect'); + }); + + test('Enabling/Disabling Pylint in root should be reflected in config settings', async () => { + const workspaceUri = Uri.file(path.join(multirootPath, 'workspace1')); + await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.WorkspaceFolder, 'pylintEnabled', undefined); + await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.Workspace, 'pylintEnabled', true); + let settings = PythonSettings.getInstance(workspaceUri); + assert.equal(settings.linting.pylintEnabled, true, 'Pylint not enabled when it should be'); + + await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.Workspace, 'pylintEnabled', false); + settings = PythonSettings.getInstance(workspaceUri); + assert.equal(settings.linting.pylintEnabled, false, 'Pylint enabled when it should not be'); + }); + + test('Enabling/Disabling Pylint in root and workspace should be reflected in config settings', async () => { + const workspaceUri = Uri.file(path.join(multirootPath, 'workspace1')); + + await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.WorkspaceFolder, 'pylintEnabled', false); + await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.Workspace, 'pylintEnabled', true); + + let cfgSetting = PythonSettings.getInstance(workspaceUri); + assert.equal(cfgSetting.linting.pylintEnabled, false, 'Workspace folder pylint setting is true when it should not be'); + PythonSettings.dispose(); + + await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.WorkspaceFolder, 'pylintEnabled', true); + await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.Workspace, 'pylintEnabled', false); + + cfgSetting = PythonSettings.getInstance(workspaceUri); + assert.equal(cfgSetting.linting.pylintEnabled, true, 'Workspace folder pylint setting is false when it should not be'); + }); + + test('Enabling/Disabling Pylint in root should be reflected in config settings when opening a document', async () => { + const workspaceUri = Uri.file(path.join(multirootPath, 'workspace1')); + const fileToOpen = path.join(multirootPath, 'workspace1', 'file.py'); + + await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.Workspace, 'pylintEnabled', false); + await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.WorkspaceFolder, 'pylintEnabled', true); + let document = await workspace.openTextDocument(fileToOpen); + let cfg = PythonSettings.getInstance(document.uri); + assert.equal(cfg.linting.pylintEnabled, true, 'Pylint should be enabled in workspace'); + PythonSettings.dispose(); + + await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.Workspace, 'pylintEnabled', true); + await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.WorkspaceFolder, 'pylintEnabled', false); + document = await workspace.openTextDocument(fileToOpen); + cfg = PythonSettings.getInstance(document.uri); + assert.equal(cfg.linting.pylintEnabled, false, 'Pylint should not be enabled in workspace'); + }); + + test('Enabling/Disabling Pylint in root should be reflected in config settings when opening a document', async () => { + const workspaceUri = Uri.file(path.join(multirootPath, 'workspace1')); + const fileToOpen = path.join(multirootPath, 'workspace1', 'file.py'); + + await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.Workspace, 'pylintEnabled', false); + await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.WorkspaceFolder, 'pylintEnabled', true); + let document = await workspace.openTextDocument(fileToOpen); + let cfg = PythonSettings.getInstance(document.uri); + assert.equal(cfg.linting.pylintEnabled, true, 'Pylint should be enabled in workspace'); + PythonSettings.dispose(); + + await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.Workspace, 'pylintEnabled', true); + await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.WorkspaceFolder, 'pylintEnabled', false); + document = await workspace.openTextDocument(fileToOpen); + cfg = PythonSettings.getInstance(document.uri); + assert.equal(cfg.linting.pylintEnabled, false, 'Pylint should not be enabled in workspace'); + }); + + // tslint:disable-next-line:no-invalid-template-strings + test('${workspaceFolder} variable in settings should be replaced with the right value', async () => { + const workspace2Uri = Uri.file(path.join(multirootPath, 'workspace2')); + let fileToOpen = path.join(workspace2Uri.fsPath, 'file.py'); + + let document = await workspace.openTextDocument(fileToOpen); + let cfg = PythonSettings.getInstance(document.uri); + assert.equal(path.dirname(cfg.workspaceSymbols.tagFilePath), workspace2Uri.fsPath, 'ctags file path for workspace2 is incorrect'); + assert.equal(path.basename(cfg.workspaceSymbols.tagFilePath), 'workspace2.tags.file', 'ctags file name for workspace2 is incorrect'); + PythonSettings.dispose(); + + const workspace3Uri = Uri.file(path.join(multirootPath, 'workspace3')); + fileToOpen = path.join(workspace3Uri.fsPath, 'file.py'); + + document = await workspace.openTextDocument(fileToOpen); + cfg = PythonSettings.getInstance(document.uri); + assert.equal(path.dirname(cfg.workspaceSymbols.tagFilePath), workspace3Uri.fsPath, 'ctags file path for workspace3 is incorrect'); + assert.equal(path.basename(cfg.workspaceSymbols.tagFilePath), 'workspace3.tags.file', 'ctags file name for workspace3 is incorrect'); + PythonSettings.dispose(); + }); }); - test('Enabling/Disabling Pylint in root should be reflected in config settings', async () => { - const workspaceUri = Uri.file(path.join(multirootPath, 'workspace1')); - await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.WorkspaceFolder, 'pylintEnabled', undefined); - await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.Workspace, 'pylintEnabled', true); - let settings = PythonSettings.getInstance(workspaceUri); - assert.equal(settings.linting.pylintEnabled, true, 'Pylint not enabled when it should be'); - - await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.Workspace, 'pylintEnabled', false); - settings = PythonSettings.getInstance(workspaceUri); - assert.equal(settings.linting.pylintEnabled, false, 'Pylint enabled when it should not be'); - }); - - test('Enabling/Disabling Pylint in root and workspace should be reflected in config settings', async () => { - const workspaceUri = Uri.file(path.join(multirootPath, 'workspace1')); - - await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.WorkspaceFolder, 'pylintEnabled', false); - await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.Workspace, 'pylintEnabled', true); - - let cfgSetting = PythonSettings.getInstance(workspaceUri); - assert.equal(cfgSetting.linting.pylintEnabled, false, 'Workspace folder pylint setting is true when it should not be'); - PythonSettings.dispose(); - - await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.WorkspaceFolder, 'pylintEnabled', true); - await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.Workspace, 'pylintEnabled', false); - - cfgSetting = PythonSettings.getInstance(workspaceUri); - assert.equal(cfgSetting.linting.pylintEnabled, true, 'Workspace folder pylint setting is false when it should not be'); - }); - - test('Enabling/Disabling Pylint in root should be reflected in config settings when opening a document', async () => { - const workspaceUri = Uri.file(path.join(multirootPath, 'workspace1')); - const fileToOpen = path.join(multirootPath, 'workspace1', 'file.py'); - - await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.Workspace, 'pylintEnabled', false); - await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.WorkspaceFolder, 'pylintEnabled', true); - let document = await workspace.openTextDocument(fileToOpen); - let cfg = PythonSettings.getInstance(document.uri); - assert.equal(cfg.linting.pylintEnabled, true, 'Pylint should be enabled in workspace'); - PythonSettings.dispose(); - - await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.Workspace, 'pylintEnabled', true); - await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.WorkspaceFolder, 'pylintEnabled', false); - document = await workspace.openTextDocument(fileToOpen); - cfg = PythonSettings.getInstance(document.uri); - assert.equal(cfg.linting.pylintEnabled, false, 'Pylint should not be enabled in workspace'); - }); - - test('Enabling/Disabling Pylint in root should be reflected in config settings when opening a document', async () => { - const workspaceUri = Uri.file(path.join(multirootPath, 'workspace1')); - const fileToOpen = path.join(multirootPath, 'workspace1', 'file.py'); - - await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.Workspace, 'pylintEnabled', false); - await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.WorkspaceFolder, 'pylintEnabled', true); - let document = await workspace.openTextDocument(fileToOpen); - let cfg = PythonSettings.getInstance(document.uri); - assert.equal(cfg.linting.pylintEnabled, true, 'Pylint should be enabled in workspace'); - PythonSettings.dispose(); - - await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.Workspace, 'pylintEnabled', true); - await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.WorkspaceFolder, 'pylintEnabled', false); - document = await workspace.openTextDocument(fileToOpen); - cfg = PythonSettings.getInstance(document.uri); - assert.equal(cfg.linting.pylintEnabled, false, 'Pylint should not be enabled in workspace'); - }); - - // tslint:disable-next-line:no-invalid-template-strings - test('${workspaceFolder} variable in settings should be replaced with the right value', async () => { - const workspace2Uri = Uri.file(path.join(multirootPath, 'workspace2')); - let fileToOpen = path.join(workspace2Uri.fsPath, 'file.py'); - - let document = await workspace.openTextDocument(fileToOpen); - let cfg = PythonSettings.getInstance(document.uri); - assert.equal(path.dirname(cfg.workspaceSymbols.tagFilePath), workspace2Uri.fsPath, 'ctags file path for workspace2 is incorrect'); - assert.equal(path.basename(cfg.workspaceSymbols.tagFilePath), 'workspace2.tags.file', 'ctags file name for workspace2 is incorrect'); - PythonSettings.dispose(); - - const workspace3Uri = Uri.file(path.join(multirootPath, 'workspace3')); - fileToOpen = path.join(workspace3Uri.fsPath, 'file.py'); - - document = await workspace.openTextDocument(fileToOpen); - cfg = PythonSettings.getInstance(document.uri); - assert.equal(path.dirname(cfg.workspaceSymbols.tagFilePath), workspace3Uri.fsPath, 'ctags file path for workspace3 is incorrect'); - assert.equal(path.basename(cfg.workspaceSymbols.tagFilePath), 'workspace3.tags.file', 'ctags file name for workspace3 is incorrect'); - PythonSettings.dispose(); - }); -}); +} diff --git a/src/test/linters/lint.commands.test.ts b/src/test/linters/lint.commands.test.ts index 5cac8d6f997f..7c2907d064a8 100644 --- a/src/test/linters/lint.commands.test.ts +++ b/src/test/linters/lint.commands.test.ts @@ -30,7 +30,7 @@ suite('Linting - Linter Selector', () => { initializeServices(); }); suiteTeardown(closeActiveWindows); - teardown(async () => await closeActiveWindows()); + teardown(async () => closeActiveWindows()); function initializeServices() { const cont = new Container(); @@ -105,7 +105,7 @@ suite('Linting - Linter Selector', () => { assert.equal(options!.matchOnDescription, true, 'Quick pick options are incorrect'); assert.equal(options!.matchOnDetail, true, 'Quick pick options are incorrect'); assert.equal(options!.placeHolder, `current: ${current}`, 'Quick pick current option is incorrect'); - assert.equal(lm.isLintingEnabled(undefined), enable, 'Linting selector did not change linting on/off flag'); + assert.equal(await lm.isLintingEnabled(false, undefined), enable, 'Linting selector did not change linting on/off flag'); } async function selectLinterAsync(products: Product[]): Promise { @@ -129,7 +129,7 @@ suite('Linting - Linter Selector', () => { await lm.setActiveLintersAsync(products); let current: string; - let activeLinters = lm.getActiveLinters(); + let activeLinters = await lm.getActiveLinters(false); switch (activeLinters.length) { case 0: current = 'none'; @@ -154,7 +154,7 @@ suite('Linting - Linter Selector', () => { assert.equal(options!.matchOnDetail, true, 'Quick pick options are incorrect'); assert.equal(options!.placeHolder, `current: ${current}`, 'Quick pick current option is incorrect'); - activeLinters = lm.getActiveLinters(); + activeLinters = await lm.getActiveLinters(false); assert.equal(activeLinters.length, 1, 'Linting selector did not change active linter'); assert.equal(activeLinters[0].product, Product.pylint, 'Linting selector did not change to pylint'); diff --git a/src/test/linters/lint.manager.test.ts b/src/test/linters/lint.manager.test.ts index d8d62ad25f2c..2d2643c7937d 100644 --- a/src/test/linters/lint.manager.test.ts +++ b/src/test/linters/lint.manager.test.ts @@ -81,15 +81,15 @@ suite('Linting - Manager', () => { test('Enable/disable linting', async () => { await lm.enableLintingAsync(false); - assert.equal(lm.isLintingEnabled(), false, 'Linting not disabled'); + assert.equal(lm.isLintingEnabled(false), false, 'Linting not disabled'); await lm.enableLintingAsync(true); - assert.equal(lm.isLintingEnabled(), true, 'Linting not enabled'); + assert.equal(lm.isLintingEnabled(false), true, 'Linting not enabled'); }); test('Set single linter', async () => { for (const linter of lm.getAllLinterInfos()) { await lm.setActiveLintersAsync([linter.product]); - const selected = lm.getActiveLinters(); + const selected = await lm.getActiveLinters(false); assert.notEqual(selected.length, 0, 'Current linter is undefined'); assert.equal(linter!.id, selected![0].id, `Selected linter ${selected} does not match requested ${linter.id}`); } @@ -97,18 +97,18 @@ suite('Linting - Manager', () => { test('Set multiple linters', async () => { await lm.setActiveLintersAsync([Product.flake8, Product.pydocstyle]); - const selected = lm.getActiveLinters(); + const selected = await lm.getActiveLinters(false); assert.equal(selected.length, 2, 'Selected linters lengths does not match'); assert.equal(Product.flake8, selected[0].product, `Selected linter ${selected[0].id} does not match requested 'flake8'`); assert.equal(Product.pydocstyle, selected[1].product, `Selected linter ${selected[1].id} does not match requested 'pydocstyle'`); }); test('Try setting unsupported linter', async () => { - const before = lm.getActiveLinters(); + const before = await lm.getActiveLinters(false); assert.notEqual(before, undefined, 'Current/before linter is undefined'); await lm.setActiveLintersAsync([Product.nosetest]); - const after = lm.getActiveLinters(); + const after = await lm.getActiveLinters(false); assert.notEqual(after, undefined, 'Current/after linter is undefined'); assert.equal(after![0].id, before![0].id, 'Should not be able to set unsupported linter'); diff --git a/src/test/linters/lint.test.ts b/src/test/linters/lint.test.ts index 4c7f57c242a7..4b0310dbfdac 100644 --- a/src/test/linters/lint.test.ts +++ b/src/test/linters/lint.test.ts @@ -163,7 +163,7 @@ suite('Linting - General Tests', () => { await linterManager.setActiveLintersAsync([product]); await linterManager.enableLintingAsync(enabled); - const linter = linterManager.createLinter(product, output, ioc.serviceContainer); + const linter = await linterManager.createLinter(product, output, ioc.serviceContainer); const messages = await linter.lint(document, cancelToken.token); if (enabled) { @@ -211,7 +211,7 @@ suite('Linting - General Tests', () => { const document = await workspace.openTextDocument(pythonFile); await linterManager.setActiveLintersAsync([product], document.uri); - const linter = linterManager.createLinter(product, outputChannel, ioc.serviceContainer); + const linter = await linterManager.createLinter(product, outputChannel, ioc.serviceContainer); const messages = await linter.lint(document, cancelToken.token); if (messagesToBeReceived.length === 0) { @@ -260,11 +260,11 @@ suite('Linting - General Tests', () => { }); // tslint:disable-next-line:no-function-expression test('Multiple linters', async function () { -// Unreliable test being skipped until we can sort it out. See gh-2609. -// - Fails about 1/3 of runs on Windows -// - Symptom: lintingEngine::lintOpenPythonFiles returns values *after* command await resolves in lint.tests -// - lintOpenPythonFiles returns 3 sets of values, not what I expect (1). -// - Haven't yet found a way to await on this properly. + // Unreliable test being skipped until we can sort it out. See gh-2609. + // - Fails about 1/3 of runs on Windows + // - Symptom: lintingEngine::lintOpenPythonFiles returns values *after* command await resolves in lint.tests + // - lintOpenPythonFiles returns 3 sets of values, not what I expect (1). + // - Haven't yet found a way to await on this properly. const skipped = true; if (skipped) { // tslint:disable-next-line:no-invalid-this @@ -295,7 +295,7 @@ suite('Linting - General Tests', () => { const document = await workspace.openTextDocument(pythonFile); await linterManager.setActiveLintersAsync([product], document.uri); - const linter = linterManager.createLinter(product, outputChannel, ioc.serviceContainer); + const linter = await linterManager.createLinter(product, outputChannel, ioc.serviceContainer); const messages = await linter.lint(document, cancelToken.token); assert.equal(messages.length, messageCountToBeReceived, 'Expected number of lint errors does not match lint error count'); diff --git a/src/test/linters/lintengine.test.ts b/src/test/linters/lintengine.test.ts index 92bb25d17ba4..f4350c4b197e 100644 --- a/src/test/linters/lintengine.test.ts +++ b/src/test/linters/lintengine.test.ts @@ -47,7 +47,7 @@ suite('Linting - LintingEngine', () => { serviceContainer.setup(c => c.get(TypeMoq.It.isValue(IOutputChannel), TypeMoq.It.isValue(STANDARD_OUTPUT_CHANNEL))).returns(() => outputChannel.object); lintManager = TypeMoq.Mock.ofType(); - lintManager.setup(x => x.isLintingEnabled(TypeMoq.It.isAny())).returns(() => true); + lintManager.setup(x => x.isLintingEnabled(TypeMoq.It.isAny())).returns(async () => true); serviceContainer.setup(c => c.get(TypeMoq.It.isValue(ILinterManager), TypeMoq.It.isAny())).returns(() => lintManager.object); lintingEngine = new LintingEngine(serviceContainer.object); @@ -59,7 +59,7 @@ suite('Linting - LintingEngine', () => { try { lintingEngine.lintDocument(doc, 'auto').ignoreErrors(); } catch { - lintManager.verify(l => l.isLintingEnabled(TypeMoq.It.isValue(doc.uri)), TypeMoq.Times.once()); + lintManager.verify(l => l.isLintingEnabled(TypeMoq.It.isAny(), TypeMoq.It.isValue(doc.uri)), TypeMoq.Times.once()); } }); test('Ensure document.uri is passed into createLinter', () => { From 840f40a59dfa9f2df85e86270f0d167e6070034e Mon Sep 17 00:00:00 2001 From: Derek Keeler Date: Fri, 19 Oct 2018 10:35:50 -0700 Subject: [PATCH 02/16] PR suggestions/corrections - Revert removal of skip() from test suite. - Fix naming of a few methods - Update method documentation to jsdocs format --- src/client/linters/linterManager.ts | 39 +- src/test/common.ts | 1 + .../common/configSettings.multiroot.test.ts | 338 +++++++++--------- 3 files changed, 198 insertions(+), 180 deletions(-) diff --git a/src/client/linters/linterManager.ts b/src/client/linters/linterManager.ts index 24e1d01ff44d..c95c2555a4f8 100644 --- a/src/client/linters/linterManager.ts +++ b/src/client/linters/linterManager.ts @@ -80,10 +80,17 @@ export class LinterManager implements ILinterManager { await this.configService.updateSetting(`linting.${this.lintingEnabledSettingName}`, enable, resource); } - /// Check if it is possible to enable an otherwise-unconfigured linter in the current - /// workspace, and if so ask the user if they want that linter configured explicitly. - /// Return true if a configuration change was made. - public async notifyUserOfInstalledLinters(linterInfo: ILinterInfo, resource?: Uri): Promise { + /** + * Check if it is possible to enable an otherwise-unconfigured linter in + * the current workspace, and if so ask the user if they want that linter + * configured explicitly. + * + * @param linterInfo The linter to check installation status. + * @param resource Context for the operation (required when in multi-root workspaces). + * + * @returns true if configuration was updated in any way, false otherwise. + */ + public async promptUserIfLinterAvailable(linterInfo: ILinterInfo, resource?: Uri): Promise { // if we've already checked during this session, don't bother again if (!this.checkedForInstalledLinters) { this.checkedForInstalledLinters = true; @@ -97,7 +104,7 @@ export class LinterManager implements ILinterManager { } // Has the linter in question has been configured explicitly? If so, no need to continue. - if (!this.isLinterUnset(linterInfo, resource)) { + if (!this.isLinterUsingDefaultConfiguration(linterInfo, resource)) { return false; } @@ -144,8 +151,12 @@ export class LinterManager implements ILinterManager { return false; } - /// Check if the linter itself is available in the workspace's Python environment or not. - /// Return true if the linter is present. + /** + * Check if the linter itself is available in the workspace's Python environment or not. + * + * @param linterProduct Linter to check for availability in the current workspace environment. + * @param resource Context information for workspace. + */ public async isLinterAvailable(linterProduct: Product, resource?: Uri): Promise { const outputChannel = this.serviceContainer.get(IOutputChannel, STANDARD_OUTPUT_CHANNEL); const linterInstaller = new LinterInstaller(this.serviceContainer, outputChannel); @@ -153,9 +164,15 @@ export class LinterManager implements ILinterManager { return linterInstaller.isInstalled(linterProduct, resource); } - /// Check if the given linter has been configured by the user in this workspace or not. - /// Return true if no explicit setting for the linter has been made. - public isLinterUnset(linterInfo: ILinterInfo, resource?: Uri) { + /** + * Check if the given linter has been configured by the user in this workspace or not. + * + * @param linterInfo Linter to check for configuration status. + * @param resource Context information. + * + * @returns true if the linter has not been configured at the user, workspace, or workspace-folder scope. false otherwise. + */ + public isLinterUsingDefaultConfiguration(linterInfo: ILinterInfo, resource?: Uri) { const workspaceConfig = this.serviceContainer.get(IWorkspaceService); const ws = workspaceConfig.getConfiguration('python.linting', resource); const pe = ws!.inspect(linterInfo.enabledSettingName); @@ -169,7 +186,7 @@ export class LinterManager implements ILinterManager { // only ask the user if they'd like to enable pylint when it is available... others may follow. const pylintInfo = this.linters.find((linter: ILinterInfo) => linter.id === 'pylint'); if (pylintInfo) { - const change = await this.notifyUserOfInstalledLinters(pylintInfo, resource); + const change = await this.promptUserIfLinterAvailable(pylintInfo, resource); if (change) { activeLinters = this.linters.filter(x => x.isEnabled(resource)); } diff --git a/src/test/common.ts b/src/test/common.ts index 7258f1b7a0ee..e3ea8cf0aa0f 100644 --- a/src/test/common.ts +++ b/src/test/common.ts @@ -124,6 +124,7 @@ export async function deleteDirectory(dir: string) { await fs.remove(dir); } } + export async function deleteFile(file: string) { const exists = await fs.pathExists(file); if (exists) { diff --git a/src/test/common/configSettings.multiroot.test.ts b/src/test/common/configSettings.multiroot.test.ts index 00b4e2d2d702..1f3190995858 100644 --- a/src/test/common/configSettings.multiroot.test.ts +++ b/src/test/common/configSettings.multiroot.test.ts @@ -7,178 +7,178 @@ import { closeActiveWindows, initialize, initializeTest, IS_MULTI_ROOT_TEST } fr const multirootPath = path.join(__dirname, '..', '..', '..', 'src', 'testMultiRootWkspc'); -if (IS_MULTI_ROOT_TEST) { - - // tslint:disable-next-line:max-func-body-length - suite('Multiroot Config Settings', () => { - suiteSetup(async () => { - await clearPythonPathInWorkspaceFolder(Uri.file(path.join(multirootPath, 'workspace1'))); - await initialize(); - }); - setup(initializeTest); - suiteTeardown(closeActiveWindows); - teardown(async () => { - await closeActiveWindows(); - await clearPythonPathInWorkspaceFolder(Uri.file(path.join(multirootPath, 'workspace1'))); - await initializeTest(); - }); - - async function enableDisableLinterSetting(resource: Uri, configTarget: ConfigurationTarget, setting: string, enabled: boolean | undefined): Promise { - const settings = workspace.getConfiguration('python.linting', resource); - const cfgValue = settings.inspect(setting); - if (configTarget === ConfigurationTarget.Workspace && cfgValue && cfgValue.workspaceValue === enabled) { - return; - } - if (configTarget === ConfigurationTarget.WorkspaceFolder && cfgValue && cfgValue.workspaceFolderValue === enabled) { - return; - } - await settings.update(setting, enabled, configTarget); - PythonSettings.dispose(); +// tslint:disable-next-line:max-func-body-length +suite('Multiroot Config Settings', () => { + suiteSetup(async function () { + if (!IS_MULTI_ROOT_TEST) { + // tslint:disable-next-line:no-invalid-this + this.skip(); } + await clearPythonPathInWorkspaceFolder(Uri.file(path.join(multirootPath, 'workspace1'))); + await initialize(); + }); + setup(initializeTest); + suiteTeardown(closeActiveWindows); + teardown(async () => { + await closeActiveWindows(); + await clearPythonPathInWorkspaceFolder(Uri.file(path.join(multirootPath, 'workspace1'))); + await initializeTest(); + }); - test('Workspace folder should inherit Python Path from workspace root', async () => { - const workspaceUri = Uri.file(path.join(multirootPath, 'workspace1')); - let settings = workspace.getConfiguration('python', workspaceUri); - const pythonPath = `x${new Date().getTime()}`; - await settings.update('pythonPath', pythonPath, ConfigurationTarget.Workspace); - const value = settings.inspect('pythonPath'); - if (value && typeof value.workspaceFolderValue === 'string') { - await settings.update('pythonPath', undefined, ConfigurationTarget.WorkspaceFolder); - } - settings = workspace.getConfiguration('python', workspaceUri); - PythonSettings.dispose(); - const cfgSetting = PythonSettings.getInstance(workspaceUri); - assert.equal(cfgSetting.pythonPath, pythonPath, 'Python Path not inherited from workspace'); - }); - - test('Workspace folder should not inherit Python Path from workspace root', async () => { - const workspaceUri = Uri.file(path.join(multirootPath, 'workspace1')); - const settings = workspace.getConfiguration('python', workspaceUri); - const pythonPath = `x${new Date().getTime()}`; - await settings.update('pythonPath', pythonPath, ConfigurationTarget.Workspace); - const privatePythonPath = `x${new Date().getTime()}`; - await settings.update('pythonPath', privatePythonPath, ConfigurationTarget.WorkspaceFolder); - - const cfgSetting = PythonSettings.getInstance(workspaceUri); - assert.equal(cfgSetting.pythonPath, privatePythonPath, 'Python Path for workspace folder is incorrect'); - }); - - test('Workspace folder should inherit Python Path from workspace root when opening a document', async () => { - const workspaceUri = Uri.file(path.join(multirootPath, 'workspace1')); - const fileToOpen = path.join(multirootPath, 'workspace1', 'file.py'); - - const settings = workspace.getConfiguration('python', workspaceUri); - const pythonPath = `x${new Date().getTime()}`; - await settings.update('pythonPath', pythonPath, ConfigurationTarget.Workspace); - // Update workspace folder to something else so it gets refreshed. - await settings.update('pythonPath', `x${new Date().getTime()}`, ConfigurationTarget.WorkspaceFolder); + async function enableDisableLinterSetting(resource: Uri, configTarget: ConfigurationTarget, setting: string, enabled: boolean | undefined): Promise { + const settings = workspace.getConfiguration('python.linting', resource); + const cfgValue = settings.inspect(setting); + if (configTarget === ConfigurationTarget.Workspace && cfgValue && cfgValue.workspaceValue === enabled) { + return; + } + if (configTarget === ConfigurationTarget.WorkspaceFolder && cfgValue && cfgValue.workspaceFolderValue === enabled) { + return; + } + await settings.update(setting, enabled, configTarget); + PythonSettings.dispose(); + } + + test('Workspace folder should inherit Python Path from workspace root', async () => { + const workspaceUri = Uri.file(path.join(multirootPath, 'workspace1')); + let settings = workspace.getConfiguration('python', workspaceUri); + const pythonPath = `x${new Date().getTime()}`; + await settings.update('pythonPath', pythonPath, ConfigurationTarget.Workspace); + const value = settings.inspect('pythonPath'); + if (value && typeof value.workspaceFolderValue === 'string') { await settings.update('pythonPath', undefined, ConfigurationTarget.WorkspaceFolder); + } + settings = workspace.getConfiguration('python', workspaceUri); + PythonSettings.dispose(); + const cfgSetting = PythonSettings.getInstance(workspaceUri); + assert.equal(cfgSetting.pythonPath, pythonPath, 'Python Path not inherited from workspace'); + }); + + test('Workspace folder should not inherit Python Path from workspace root', async () => { + const workspaceUri = Uri.file(path.join(multirootPath, 'workspace1')); + const settings = workspace.getConfiguration('python', workspaceUri); + const pythonPath = `x${new Date().getTime()}`; + await settings.update('pythonPath', pythonPath, ConfigurationTarget.Workspace); + const privatePythonPath = `x${new Date().getTime()}`; + await settings.update('pythonPath', privatePythonPath, ConfigurationTarget.WorkspaceFolder); + + const cfgSetting = PythonSettings.getInstance(workspaceUri); + assert.equal(cfgSetting.pythonPath, privatePythonPath, 'Python Path for workspace folder is incorrect'); + }); + + test('Workspace folder should inherit Python Path from workspace root when opening a document', async () => { + const workspaceUri = Uri.file(path.join(multirootPath, 'workspace1')); + const fileToOpen = path.join(multirootPath, 'workspace1', 'file.py'); + + const settings = workspace.getConfiguration('python', workspaceUri); + const pythonPath = `x${new Date().getTime()}`; + await settings.update('pythonPath', pythonPath, ConfigurationTarget.Workspace); + // Update workspace folder to something else so it gets refreshed. + await settings.update('pythonPath', `x${new Date().getTime()}`, ConfigurationTarget.WorkspaceFolder); + await settings.update('pythonPath', undefined, ConfigurationTarget.WorkspaceFolder); + + const document = await workspace.openTextDocument(fileToOpen); + const cfg = PythonSettings.getInstance(document.uri); + assert.equal(cfg.pythonPath, pythonPath, 'Python Path not inherited from workspace'); + }); + + test('Workspace folder should not inherit Python Path from workspace root when opening a document', async () => { + const workspaceUri = Uri.file(path.join(multirootPath, 'workspace1')); + const fileToOpen = path.join(multirootPath, 'workspace1', 'file.py'); + + const settings = workspace.getConfiguration('python', workspaceUri); + const pythonPath = `x${new Date().getTime()}`; + await settings.update('pythonPath', pythonPath, ConfigurationTarget.Workspace); + const privatePythonPath = `x${new Date().getTime()}`; + await settings.update('pythonPath', privatePythonPath, ConfigurationTarget.WorkspaceFolder); - const document = await workspace.openTextDocument(fileToOpen); - const cfg = PythonSettings.getInstance(document.uri); - assert.equal(cfg.pythonPath, pythonPath, 'Python Path not inherited from workspace'); - }); - - test('Workspace folder should not inherit Python Path from workspace root when opening a document', async () => { - const workspaceUri = Uri.file(path.join(multirootPath, 'workspace1')); - const fileToOpen = path.join(multirootPath, 'workspace1', 'file.py'); - - const settings = workspace.getConfiguration('python', workspaceUri); - const pythonPath = `x${new Date().getTime()}`; - await settings.update('pythonPath', pythonPath, ConfigurationTarget.Workspace); - const privatePythonPath = `x${new Date().getTime()}`; - await settings.update('pythonPath', privatePythonPath, ConfigurationTarget.WorkspaceFolder); - - const document = await workspace.openTextDocument(fileToOpen); - const cfg = PythonSettings.getInstance(document.uri); - assert.equal(cfg.pythonPath, privatePythonPath, 'Python Path for workspace folder is incorrect'); - }); - - test('Enabling/Disabling Pylint in root should be reflected in config settings', async () => { - const workspaceUri = Uri.file(path.join(multirootPath, 'workspace1')); - await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.WorkspaceFolder, 'pylintEnabled', undefined); - await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.Workspace, 'pylintEnabled', true); - let settings = PythonSettings.getInstance(workspaceUri); - assert.equal(settings.linting.pylintEnabled, true, 'Pylint not enabled when it should be'); - - await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.Workspace, 'pylintEnabled', false); - settings = PythonSettings.getInstance(workspaceUri); - assert.equal(settings.linting.pylintEnabled, false, 'Pylint enabled when it should not be'); - }); - - test('Enabling/Disabling Pylint in root and workspace should be reflected in config settings', async () => { - const workspaceUri = Uri.file(path.join(multirootPath, 'workspace1')); - - await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.WorkspaceFolder, 'pylintEnabled', false); - await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.Workspace, 'pylintEnabled', true); - - let cfgSetting = PythonSettings.getInstance(workspaceUri); - assert.equal(cfgSetting.linting.pylintEnabled, false, 'Workspace folder pylint setting is true when it should not be'); - PythonSettings.dispose(); - - await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.WorkspaceFolder, 'pylintEnabled', true); - await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.Workspace, 'pylintEnabled', false); - - cfgSetting = PythonSettings.getInstance(workspaceUri); - assert.equal(cfgSetting.linting.pylintEnabled, true, 'Workspace folder pylint setting is false when it should not be'); - }); - - test('Enabling/Disabling Pylint in root should be reflected in config settings when opening a document', async () => { - const workspaceUri = Uri.file(path.join(multirootPath, 'workspace1')); - const fileToOpen = path.join(multirootPath, 'workspace1', 'file.py'); - - await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.Workspace, 'pylintEnabled', false); - await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.WorkspaceFolder, 'pylintEnabled', true); - let document = await workspace.openTextDocument(fileToOpen); - let cfg = PythonSettings.getInstance(document.uri); - assert.equal(cfg.linting.pylintEnabled, true, 'Pylint should be enabled in workspace'); - PythonSettings.dispose(); - - await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.Workspace, 'pylintEnabled', true); - await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.WorkspaceFolder, 'pylintEnabled', false); - document = await workspace.openTextDocument(fileToOpen); - cfg = PythonSettings.getInstance(document.uri); - assert.equal(cfg.linting.pylintEnabled, false, 'Pylint should not be enabled in workspace'); - }); - - test('Enabling/Disabling Pylint in root should be reflected in config settings when opening a document', async () => { - const workspaceUri = Uri.file(path.join(multirootPath, 'workspace1')); - const fileToOpen = path.join(multirootPath, 'workspace1', 'file.py'); - - await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.Workspace, 'pylintEnabled', false); - await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.WorkspaceFolder, 'pylintEnabled', true); - let document = await workspace.openTextDocument(fileToOpen); - let cfg = PythonSettings.getInstance(document.uri); - assert.equal(cfg.linting.pylintEnabled, true, 'Pylint should be enabled in workspace'); - PythonSettings.dispose(); - - await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.Workspace, 'pylintEnabled', true); - await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.WorkspaceFolder, 'pylintEnabled', false); - document = await workspace.openTextDocument(fileToOpen); - cfg = PythonSettings.getInstance(document.uri); - assert.equal(cfg.linting.pylintEnabled, false, 'Pylint should not be enabled in workspace'); - }); - - // tslint:disable-next-line:no-invalid-template-strings - test('${workspaceFolder} variable in settings should be replaced with the right value', async () => { - const workspace2Uri = Uri.file(path.join(multirootPath, 'workspace2')); - let fileToOpen = path.join(workspace2Uri.fsPath, 'file.py'); - - let document = await workspace.openTextDocument(fileToOpen); - let cfg = PythonSettings.getInstance(document.uri); - assert.equal(path.dirname(cfg.workspaceSymbols.tagFilePath), workspace2Uri.fsPath, 'ctags file path for workspace2 is incorrect'); - assert.equal(path.basename(cfg.workspaceSymbols.tagFilePath), 'workspace2.tags.file', 'ctags file name for workspace2 is incorrect'); - PythonSettings.dispose(); - - const workspace3Uri = Uri.file(path.join(multirootPath, 'workspace3')); - fileToOpen = path.join(workspace3Uri.fsPath, 'file.py'); - - document = await workspace.openTextDocument(fileToOpen); - cfg = PythonSettings.getInstance(document.uri); - assert.equal(path.dirname(cfg.workspaceSymbols.tagFilePath), workspace3Uri.fsPath, 'ctags file path for workspace3 is incorrect'); - assert.equal(path.basename(cfg.workspaceSymbols.tagFilePath), 'workspace3.tags.file', 'ctags file name for workspace3 is incorrect'); - PythonSettings.dispose(); - }); + const document = await workspace.openTextDocument(fileToOpen); + const cfg = PythonSettings.getInstance(document.uri); + assert.equal(cfg.pythonPath, privatePythonPath, 'Python Path for workspace folder is incorrect'); }); -} + test('Enabling/Disabling Pylint in root should be reflected in config settings', async () => { + const workspaceUri = Uri.file(path.join(multirootPath, 'workspace1')); + await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.WorkspaceFolder, 'pylintEnabled', undefined); + await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.Workspace, 'pylintEnabled', true); + let settings = PythonSettings.getInstance(workspaceUri); + assert.equal(settings.linting.pylintEnabled, true, 'Pylint not enabled when it should be'); + + await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.Workspace, 'pylintEnabled', false); + settings = PythonSettings.getInstance(workspaceUri); + assert.equal(settings.linting.pylintEnabled, false, 'Pylint enabled when it should not be'); + }); + + test('Enabling/Disabling Pylint in root and workspace should be reflected in config settings', async () => { + const workspaceUri = Uri.file(path.join(multirootPath, 'workspace1')); + + await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.WorkspaceFolder, 'pylintEnabled', false); + await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.Workspace, 'pylintEnabled', true); + + let cfgSetting = PythonSettings.getInstance(workspaceUri); + assert.equal(cfgSetting.linting.pylintEnabled, false, 'Workspace folder pylint setting is true when it should not be'); + PythonSettings.dispose(); + + await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.WorkspaceFolder, 'pylintEnabled', true); + await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.Workspace, 'pylintEnabled', false); + + cfgSetting = PythonSettings.getInstance(workspaceUri); + assert.equal(cfgSetting.linting.pylintEnabled, true, 'Workspace folder pylint setting is false when it should not be'); + }); + + test('Enabling/Disabling Pylint in root should be reflected in config settings when opening a document', async () => { + const workspaceUri = Uri.file(path.join(multirootPath, 'workspace1')); + const fileToOpen = path.join(multirootPath, 'workspace1', 'file.py'); + + await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.Workspace, 'pylintEnabled', false); + await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.WorkspaceFolder, 'pylintEnabled', true); + let document = await workspace.openTextDocument(fileToOpen); + let cfg = PythonSettings.getInstance(document.uri); + assert.equal(cfg.linting.pylintEnabled, true, 'Pylint should be enabled in workspace'); + PythonSettings.dispose(); + + await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.Workspace, 'pylintEnabled', true); + await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.WorkspaceFolder, 'pylintEnabled', false); + document = await workspace.openTextDocument(fileToOpen); + cfg = PythonSettings.getInstance(document.uri); + assert.equal(cfg.linting.pylintEnabled, false, 'Pylint should not be enabled in workspace'); + }); + + test('Enabling/Disabling Pylint in root should be reflected in config settings when opening a document', async () => { + const workspaceUri = Uri.file(path.join(multirootPath, 'workspace1')); + const fileToOpen = path.join(multirootPath, 'workspace1', 'file.py'); + + await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.Workspace, 'pylintEnabled', false); + await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.WorkspaceFolder, 'pylintEnabled', true); + let document = await workspace.openTextDocument(fileToOpen); + let cfg = PythonSettings.getInstance(document.uri); + assert.equal(cfg.linting.pylintEnabled, true, 'Pylint should be enabled in workspace'); + PythonSettings.dispose(); + + await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.Workspace, 'pylintEnabled', true); + await enableDisableLinterSetting(workspaceUri, ConfigurationTarget.WorkspaceFolder, 'pylintEnabled', false); + document = await workspace.openTextDocument(fileToOpen); + cfg = PythonSettings.getInstance(document.uri); + assert.equal(cfg.linting.pylintEnabled, false, 'Pylint should not be enabled in workspace'); + }); + + // tslint:disable-next-line:no-invalid-template-strings + test('${workspaceFolder} variable in settings should be replaced with the right value', async () => { + const workspace2Uri = Uri.file(path.join(multirootPath, 'workspace2')); + let fileToOpen = path.join(workspace2Uri.fsPath, 'file.py'); + + let document = await workspace.openTextDocument(fileToOpen); + let cfg = PythonSettings.getInstance(document.uri); + assert.equal(path.dirname(cfg.workspaceSymbols.tagFilePath), workspace2Uri.fsPath, 'ctags file path for workspace2 is incorrect'); + assert.equal(path.basename(cfg.workspaceSymbols.tagFilePath), 'workspace2.tags.file', 'ctags file name for workspace2 is incorrect'); + PythonSettings.dispose(); + + const workspace3Uri = Uri.file(path.join(multirootPath, 'workspace3')); + fileToOpen = path.join(workspace3Uri.fsPath, 'file.py'); + + document = await workspace.openTextDocument(fileToOpen); + cfg = PythonSettings.getInstance(document.uri); + assert.equal(path.dirname(cfg.workspaceSymbols.tagFilePath), workspace3Uri.fsPath, 'ctags file path for workspace3 is incorrect'); + assert.equal(path.basename(cfg.workspaceSymbols.tagFilePath), 'workspace3.tags.file', 'ctags file name for workspace3 is incorrect'); + PythonSettings.dispose(); + }); +}); From 16f483e95e574915fd4d512a31262de1b42bcda6 Mon Sep 17 00:00:00 2001 From: Derek Keeler Date: Fri, 19 Oct 2018 10:45:44 -0700 Subject: [PATCH 03/16] Error handling when investigating linter installation status --- src/client/linters/linterManager.ts | 32 +++++++++++++++++------------ 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/src/client/linters/linterManager.ts b/src/client/linters/linterManager.ts index c95c2555a4f8..54f65a22ba45 100644 --- a/src/client/linters/linterManager.ts +++ b/src/client/linters/linterManager.ts @@ -5,12 +5,13 @@ import { inject, injectable } from 'inversify'; import { - CancellationToken, MessageItem, OutputChannel, + CancellationToken, OutputChannel, TextDocument, Uri } from 'vscode'; import { IApplicationShell, IWorkspaceService } from '../common/application/types'; import { STANDARD_OUTPUT_CHANNEL } from '../common/constants'; import { LinterInstaller } from '../common/installer/productInstaller'; +import { error as traceError } from '../common/logger'; import { IConfigurationService, ILogger, IOutputChannel, Product @@ -25,7 +26,10 @@ import { Prospector } from './prospector'; import { PyDocStyle } from './pydocstyle'; import { PyLama } from './pylama'; import { Pylint } from './pylint'; -import { ILinter, ILinterInfo, ILinterManager, ILintMessage } from './types'; +import { + ILinter, ILinterInfo, + ILinterManager, ILintMessage +} from './types'; class DisabledLinter implements ILinter { constructor(private configService: IConfigurationService) { } @@ -123,14 +127,10 @@ export class LinterManager implements ILinterManager { public async notifyUserAndConfigureLinter(linterInfo: ILinterInfo): Promise { const appShell = this.serviceContainer.get(IApplicationShell); - class ConfigureLinterMessage implements MessageItem { - public enabled: boolean; - public title: string; - constructor() { - this.enabled = true; - this.title = ''; - } - } + type ConfigureLinterMessage = { + enabled: boolean; + title: string; + }; const optButtons: ConfigureLinterMessage[] = [ { @@ -152,16 +152,22 @@ export class LinterManager implements ILinterManager { } /** - * Check if the linter itself is available in the workspace's Python environment or not. + * Check if the linter itself is available in the workspace's Python environment or + * not. * - * @param linterProduct Linter to check for availability in the current workspace environment. + * @param linterProduct Linter to check in the current workspace environment. * @param resource Context information for workspace. */ public async isLinterAvailable(linterProduct: Product, resource?: Uri): Promise { const outputChannel = this.serviceContainer.get(IOutputChannel, STANDARD_OUTPUT_CHANNEL); const linterInstaller = new LinterInstaller(this.serviceContainer, outputChannel); - return linterInstaller.isInstalled(linterProduct, resource); + return linterInstaller.isInstalled(linterProduct, resource) + .catch((reason) => { + // report and continue, assume the linter is unavailable. + traceError(`[WARNING]: Failed to discover if linter ${linterProduct} is installed.`, reason); + return false; + }); } /** From 9367ad425fbfd1cb91e4e0d6545f2139dcee607a Mon Sep 17 00:00:00 2001 From: Derek Keeler Date: Fri, 19 Oct 2018 10:50:05 -0700 Subject: [PATCH 04/16] Simplify logic flow to getActiveLinters --- src/client/linters/linterManager.ts | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/client/linters/linterManager.ts b/src/client/linters/linterManager.ts index 54f65a22ba45..70b480bb802a 100644 --- a/src/client/linters/linterManager.ts +++ b/src/client/linters/linterManager.ts @@ -186,19 +186,14 @@ export class LinterManager implements ILinterManager { } public async getActiveLinters(checkAvailable: boolean, resource?: Uri): Promise { - let activeLinters = this.linters.filter(x => x.isEnabled(resource)); - if (checkAvailable) { // only ask the user if they'd like to enable pylint when it is available... others may follow. const pylintInfo = this.linters.find((linter: ILinterInfo) => linter.id === 'pylint'); if (pylintInfo) { - const change = await this.promptUserIfLinterAvailable(pylintInfo, resource); - if (change) { - activeLinters = this.linters.filter(x => x.isEnabled(resource)); - } + await this.promptUserIfLinterAvailable(pylintInfo, resource); } } - return activeLinters; + return this.linters.filter(x => x.isEnabled(resource)); } public async setActiveLintersAsync(products: Product[], resource?: Uri): Promise { From 189b83283e78b32ddeea088541b6c0f781153b81 Mon Sep 17 00:00:00 2001 From: Derek Keeler Date: Fri, 19 Oct 2018 10:53:25 -0700 Subject: [PATCH 05/16] Simplify name of parameters --- src/client/linters/linterManager.ts | 8 ++++---- src/client/linters/types.ts | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/client/linters/linterManager.ts b/src/client/linters/linterManager.ts index 70b480bb802a..4808866aae82 100644 --- a/src/client/linters/linterManager.ts +++ b/src/client/linters/linterManager.ts @@ -74,9 +74,9 @@ export class LinterManager implements ILinterManager { throw new Error('Invalid linter'); } - public async isLintingEnabled(checkAvailable: boolean, resource?: Uri): Promise { + public async isLintingEnabled(silent: boolean, resource?: Uri): Promise { const settings = this.configService.getSettings(resource); - const activeLintersPresent = await this.getActiveLinters(checkAvailable, resource); + const activeLintersPresent = await this.getActiveLinters(silent, resource); return (settings.linting[this.lintingEnabledSettingName] as boolean) && activeLintersPresent.length > 0; } @@ -185,8 +185,8 @@ export class LinterManager implements ILinterManager { return (pe!.globalValue === undefined && pe!.workspaceValue === undefined && pe!.workspaceFolderValue === undefined); } - public async getActiveLinters(checkAvailable: boolean, resource?: Uri): Promise { - if (checkAvailable) { + public async getActiveLinters(silent: boolean, resource?: Uri): Promise { + if (silent) { // only ask the user if they'd like to enable pylint when it is available... others may follow. const pylintInfo = this.linters.find((linter: ILinterInfo) => linter.id === 'pylint'); if (pylintInfo) { diff --git a/src/client/linters/types.ts b/src/client/linters/types.ts index cd09ac43f865..4b0af07cc6e7 100644 --- a/src/client/linters/types.ts +++ b/src/client/linters/types.ts @@ -35,8 +35,8 @@ export const ILinterManager = Symbol('ILinterManager'); export interface ILinterManager { getAllLinterInfos(): ILinterInfo[]; getLinterInfo(product: Product): ILinterInfo; - getActiveLinters(checkAvailable: boolean, resource?: vscode.Uri): Promise; - isLintingEnabled(checkAvailable: boolean, resource?: vscode.Uri): Promise; + getActiveLinters(silent: boolean, resource?: vscode.Uri): Promise; + isLintingEnabled(silent: boolean, resource?: vscode.Uri): Promise; enableLintingAsync(enable: boolean, resource?: vscode.Uri): Promise; setActiveLintersAsync(products: Product[], resource?: vscode.Uri): Promise; createLinter(product: Product, outputChannel: vscode.OutputChannel, serviceContainer: IServiceContainer, resource?: vscode.Uri): Promise; From 7e0fcbd92b47d5a02ed3a98b399a1844318f7079 Mon Sep 17 00:00:00 2001 From: Derek Keeler Date: Fri, 19 Oct 2018 12:33:46 -0700 Subject: [PATCH 06/16] Udpate project for switch of meaning in flag. - flag changed from `checkAvailable` to `silent` - affects calls to `getActiveLinters` and `isLintingEnabled` --- src/client/linters/linterCommands.ts | 4 ++-- src/client/linters/lintingEngine.ts | 4 ++-- src/client/providers/linterProvider.ts | 2 +- src/test/linters/lint.commands.test.ts | 6 +++--- src/test/linters/lint.manager.test.ts | 12 ++++++------ 5 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/client/linters/linterCommands.ts b/src/client/linters/linterCommands.ts index c5e12e4f3ba0..04db197fc9f6 100644 --- a/src/client/linters/linterCommands.ts +++ b/src/client/linters/linterCommands.ts @@ -28,7 +28,7 @@ export class LinterCommands implements vscode.Disposable { public async setLinterAsync(): Promise { const linters = this.linterManager.getAllLinterInfos(); const suggestions = linters.map(x => x.id).sort(); - const activeLinters = await this.linterManager.getActiveLinters(false, this.settingsUri); + const activeLinters = await this.linterManager.getActiveLinters(true, this.settingsUri); let current: string; switch (activeLinters.length) { @@ -64,7 +64,7 @@ export class LinterCommands implements vscode.Disposable { public async enableLintingAsync(): Promise { const options = ['on', 'off']; - const current = await this.linterManager.isLintingEnabled(false, this.settingsUri) ? options[0] : options[1]; + const current = await this.linterManager.isLintingEnabled(true, this.settingsUri) ? options[0] : options[1]; const quickPickOptions: vscode.QuickPickOptions = { matchOnDetail: true, diff --git a/src/client/linters/lintingEngine.ts b/src/client/linters/lintingEngine.ts index bca846dc4ae5..f27618c4f2a5 100644 --- a/src/client/linters/lintingEngine.ts +++ b/src/client/linters/lintingEngine.ts @@ -95,7 +95,7 @@ export class LintingEngine implements ILintingEngine { this.pendingLintings.set(document.uri.fsPath, cancelToken); - const activeLinters = await this.linterManager.getActiveLinters(true, document.uri); + const activeLinters = await this.linterManager.getActiveLinters(false, document.uri); const promises: Promise[] = activeLinters .map(async (info: ILinterInfo) => { const stopWatch = new StopWatch(); @@ -183,7 +183,7 @@ export class LintingEngine implements ILintingEngine { } private async shouldLintDocument(document: vscode.TextDocument): Promise { - if (!await this.linterManager.isLintingEnabled(true, document.uri)) { + if (!await this.linterManager.isLintingEnabled(false, document.uri)) { this.diagnosticCollection.set(document.uri, []); return false; } diff --git a/src/client/providers/linterProvider.ts b/src/client/providers/linterProvider.ts index 274df08822a5..952d955aa69d 100644 --- a/src/client/providers/linterProvider.ts +++ b/src/client/providers/linterProvider.ts @@ -89,7 +89,7 @@ export class LinterProvider implements Disposable { return; } - const linters = await this.linterManager.getActiveLinters(true, document.uri); + const linters = await this.linterManager.getActiveLinters(false, document.uri); const fileName = path.basename(document.uri.fsPath).toLowerCase(); const watchers = linters.filter((info) => info.configFileNames.indexOf(fileName) >= 0); if (watchers.length > 0) { diff --git a/src/test/linters/lint.commands.test.ts b/src/test/linters/lint.commands.test.ts index 7c2907d064a8..c9e202f1c2ac 100644 --- a/src/test/linters/lint.commands.test.ts +++ b/src/test/linters/lint.commands.test.ts @@ -105,7 +105,7 @@ suite('Linting - Linter Selector', () => { assert.equal(options!.matchOnDescription, true, 'Quick pick options are incorrect'); assert.equal(options!.matchOnDetail, true, 'Quick pick options are incorrect'); assert.equal(options!.placeHolder, `current: ${current}`, 'Quick pick current option is incorrect'); - assert.equal(await lm.isLintingEnabled(false, undefined), enable, 'Linting selector did not change linting on/off flag'); + assert.equal(await lm.isLintingEnabled(true, undefined), enable, 'Linting selector did not change linting on/off flag'); } async function selectLinterAsync(products: Product[]): Promise { @@ -129,7 +129,7 @@ suite('Linting - Linter Selector', () => { await lm.setActiveLintersAsync(products); let current: string; - let activeLinters = await lm.getActiveLinters(false); + let activeLinters = await lm.getActiveLinters(true); switch (activeLinters.length) { case 0: current = 'none'; @@ -154,7 +154,7 @@ suite('Linting - Linter Selector', () => { assert.equal(options!.matchOnDetail, true, 'Quick pick options are incorrect'); assert.equal(options!.placeHolder, `current: ${current}`, 'Quick pick current option is incorrect'); - activeLinters = await lm.getActiveLinters(false); + activeLinters = await lm.getActiveLinters(true); assert.equal(activeLinters.length, 1, 'Linting selector did not change active linter'); assert.equal(activeLinters[0].product, Product.pylint, 'Linting selector did not change to pylint'); diff --git a/src/test/linters/lint.manager.test.ts b/src/test/linters/lint.manager.test.ts index 2d2643c7937d..3d0abb77d7be 100644 --- a/src/test/linters/lint.manager.test.ts +++ b/src/test/linters/lint.manager.test.ts @@ -81,15 +81,15 @@ suite('Linting - Manager', () => { test('Enable/disable linting', async () => { await lm.enableLintingAsync(false); - assert.equal(lm.isLintingEnabled(false), false, 'Linting not disabled'); + assert.equal(lm.isLintingEnabled(true), false, 'Linting not disabled'); await lm.enableLintingAsync(true); - assert.equal(lm.isLintingEnabled(false), true, 'Linting not enabled'); + assert.equal(lm.isLintingEnabled(true), true, 'Linting not enabled'); }); test('Set single linter', async () => { for (const linter of lm.getAllLinterInfos()) { await lm.setActiveLintersAsync([linter.product]); - const selected = await lm.getActiveLinters(false); + const selected = await lm.getActiveLinters(true); assert.notEqual(selected.length, 0, 'Current linter is undefined'); assert.equal(linter!.id, selected![0].id, `Selected linter ${selected} does not match requested ${linter.id}`); } @@ -97,18 +97,18 @@ suite('Linting - Manager', () => { test('Set multiple linters', async () => { await lm.setActiveLintersAsync([Product.flake8, Product.pydocstyle]); - const selected = await lm.getActiveLinters(false); + const selected = await lm.getActiveLinters(true); assert.equal(selected.length, 2, 'Selected linters lengths does not match'); assert.equal(Product.flake8, selected[0].product, `Selected linter ${selected[0].id} does not match requested 'flake8'`); assert.equal(Product.pydocstyle, selected[1].product, `Selected linter ${selected[1].id} does not match requested 'pydocstyle'`); }); test('Try setting unsupported linter', async () => { - const before = await lm.getActiveLinters(false); + const before = await lm.getActiveLinters(true); assert.notEqual(before, undefined, 'Current/before linter is undefined'); await lm.setActiveLintersAsync([Product.nosetest]); - const after = await lm.getActiveLinters(false); + const after = await lm.getActiveLinters(true); assert.notEqual(after, undefined, 'Current/after linter is undefined'); assert.equal(after![0].id, before![0].id, 'Should not be able to set unsupported linter'); From 7c890bbcf47c9e92f3a1184ad54e73266f1192eb Mon Sep 17 00:00:00 2001 From: Derek Keeler Date: Fri, 19 Oct 2018 12:54:47 -0700 Subject: [PATCH 07/16] Relocate logic that checks for and enables available linter. - new class `AvailableLinterActivator` - DI enable the new class - correct some issues with DI usage in the original logic --- src/client/linters/linterAvailability.ts | 109 +++++++++++++++++ src/client/linters/linterManager.ts | 146 +++++------------------ src/client/linters/serviceRegistry.ts | 6 +- src/client/linters/types.ts | 5 + 4 files changed, 146 insertions(+), 120 deletions(-) create mode 100644 src/client/linters/linterAvailability.ts diff --git a/src/client/linters/linterAvailability.ts b/src/client/linters/linterAvailability.ts new file mode 100644 index 000000000000..80cce0dc8700 --- /dev/null +++ b/src/client/linters/linterAvailability.ts @@ -0,0 +1,109 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +'use strict'; + +import { inject, injectable } from 'inversify'; +import { Uri } from 'vscode'; +import { IApplicationShell, IWorkspaceService } from '../common/application/types'; +import { error as traceError } from '../common/logger'; +import { IInstaller, Product } from '../common/types'; +import { IAvailableLinterActivator, ILinterInfo } from './types'; + +@injectable() +export class AvailableLinterActivator implements IAvailableLinterActivator { + constructor( + @inject(IApplicationShell) private appShell: IApplicationShell, + @inject(IInstaller) private installer: IInstaller, + @inject(IWorkspaceService) private workspaceConfig: IWorkspaceService + ) { } + + /** + * Check if it is possible to enable an otherwise-unconfigured linter in + * the current workspace, and if so ask the user if they want that linter + * configured explicitly. + * + * @param linterInfo The linter to check installation status. + * @param resource Context for the operation (required when in multi-root workspaces). + * + * @returns true if configuration was updated in any way, false otherwise. + */ + public async promptIfLinterAvailable(linterInfo: ILinterInfo, resource?: Uri): Promise { + + // Has the linter in question has been configured explicitly? If so, no need to continue. + if (!this.isLinterUsingDefaultConfiguration(linterInfo, resource)) { + return false; + } + + // Is the linter available in the current workspace? + if (await this.isLinterAvailable(linterInfo.product, resource)) { + + // great, it is - ask the user if they'd like to enable it. + return this.promptToConfigureAvailableLinter(linterInfo); + } + return false; + } + + /** + * Raise a dialog asking the user if they would like to explicitly configure a + * linter or not in their current workspace. + * + * @param linterInfo The linter to ask the user to enable or not. + * + * @returns true if the user requested a configuration change, false otherwise. + */ + public async promptToConfigureAvailableLinter(linterInfo: ILinterInfo): Promise { + type ConfigureLinterMessage = { + enabled: boolean; + title: string; + }; + + const optButtons: ConfigureLinterMessage[] = [ + { + title: `Enable ${linterInfo.id}`, + enabled: true + }, + { + title: `Disable ${linterInfo.id}`, + enabled: false + } + ]; + const pick = await this.appShell.showInformationMessage(`Linter ${linterInfo.id} is available but not enabled.`, ...optButtons); + if (pick) { + await linterInfo.enableAsync(pick.enabled); + return true; + } + + return false; + } + + /** + * Check if the linter itself is available in the workspace's Python environment or + * not. + * + * @param linterProduct Linter to check in the current workspace environment. + * @param resource Context information for workspace. + */ + public async isLinterAvailable(linterProduct: Product, resource?: Uri): Promise { + return this.installer.isInstalled(linterProduct, resource) + .catch((reason) => { + // report and continue, assume the linter is unavailable. + traceError(`[WARNING]: Failed to discover if linter ${linterProduct} is installed.`, reason); + return false; + }); + } + + /** + * Check if the given linter has been configured by the user in this workspace or not. + * + * @param linterInfo Linter to check for configuration status. + * @param resource Context information. + * + * @returns true if the linter has not been configured at the user, workspace, or workspace-folder scope. false otherwise. + */ + public isLinterUsingDefaultConfiguration(linterInfo: ILinterInfo, resource?: Uri) { + const ws = this.workspaceConfig.getConfiguration('python.linting', resource); + const pe = ws!.inspect(linterInfo.enabledSettingName); + return (pe!.globalValue === undefined && pe!.workspaceValue === undefined && pe!.workspaceFolderValue === undefined); + } +} diff --git a/src/client/linters/linterManager.ts b/src/client/linters/linterManager.ts index 4808866aae82..719e2261daaa 100644 --- a/src/client/linters/linterManager.ts +++ b/src/client/linters/linterManager.ts @@ -5,16 +5,10 @@ import { inject, injectable } from 'inversify'; import { - CancellationToken, OutputChannel, - TextDocument, Uri + CancellationToken, OutputChannel, TextDocument, Uri } from 'vscode'; -import { IApplicationShell, IWorkspaceService } from '../common/application/types'; -import { STANDARD_OUTPUT_CHANNEL } from '../common/constants'; -import { LinterInstaller } from '../common/installer/productInstaller'; -import { error as traceError } from '../common/logger'; import { - IConfigurationService, ILogger, - IOutputChannel, Product + IConfigurationService, ILogger, Product } from '../common/types'; import { IServiceContainer } from '../ioc/types'; import { Bandit } from './bandit'; @@ -27,8 +21,7 @@ import { PyDocStyle } from './pydocstyle'; import { PyLama } from './pylama'; import { Pylint } from './pylint'; import { - ILinter, ILinterInfo, - ILinterManager, ILintMessage + IAvailableLinterActivator, ILinter, ILinterInfo, ILinterManager, ILintMessage } from './types'; class DisabledLinter implements ILinter { @@ -84,120 +77,15 @@ export class LinterManager implements ILinterManager { await this.configService.updateSetting(`linting.${this.lintingEnabledSettingName}`, enable, resource); } - /** - * Check if it is possible to enable an otherwise-unconfigured linter in - * the current workspace, and if so ask the user if they want that linter - * configured explicitly. - * - * @param linterInfo The linter to check installation status. - * @param resource Context for the operation (required when in multi-root workspaces). - * - * @returns true if configuration was updated in any way, false otherwise. - */ - public async promptUserIfLinterAvailable(linterInfo: ILinterInfo, resource?: Uri): Promise { - // if we've already checked during this session, don't bother again - if (!this.checkedForInstalledLinters) { - this.checkedForInstalledLinters = true; - } else { - return false; - } - - // If linting is disabled, we are finished. - if (!await this.isLintingEnabled(false, resource)) { - return false; - } - - // Has the linter in question has been configured explicitly? If so, no need to continue. - if (!this.isLinterUsingDefaultConfiguration(linterInfo, resource)) { - return false; - } - - // Is the linter available in the current workspace? - if (await this.isLinterAvailable(linterInfo.product, resource)) { - // ...ask the user if they would like to enable it. - return this.notifyUserAndConfigureLinter(linterInfo); - } else { - return false; - } - - } - - /// Raise a dialog asking the user if they would like to explicitly configure a linter or not. - /// Return true if a config change was made. - public async notifyUserAndConfigureLinter(linterInfo: ILinterInfo): Promise { - const appShell = this.serviceContainer.get(IApplicationShell); - - type ConfigureLinterMessage = { - enabled: boolean; - title: string; - }; - - const optButtons: ConfigureLinterMessage[] = [ - { - title: `Enable ${linterInfo.id}`, - enabled: true - }, - { - title: `Disable ${linterInfo.id}`, - enabled: false - } - ]; - const pick = await appShell.showInformationMessage(`Linter ${linterInfo.id} is available but not enabled.`, ...optButtons); - if (pick) { - await linterInfo.enableAsync(pick.enabled); - return true; - } - - return false; - } - - /** - * Check if the linter itself is available in the workspace's Python environment or - * not. - * - * @param linterProduct Linter to check in the current workspace environment. - * @param resource Context information for workspace. - */ - public async isLinterAvailable(linterProduct: Product, resource?: Uri): Promise { - const outputChannel = this.serviceContainer.get(IOutputChannel, STANDARD_OUTPUT_CHANNEL); - const linterInstaller = new LinterInstaller(this.serviceContainer, outputChannel); - - return linterInstaller.isInstalled(linterProduct, resource) - .catch((reason) => { - // report and continue, assume the linter is unavailable. - traceError(`[WARNING]: Failed to discover if linter ${linterProduct} is installed.`, reason); - return false; - }); - } - - /** - * Check if the given linter has been configured by the user in this workspace or not. - * - * @param linterInfo Linter to check for configuration status. - * @param resource Context information. - * - * @returns true if the linter has not been configured at the user, workspace, or workspace-folder scope. false otherwise. - */ - public isLinterUsingDefaultConfiguration(linterInfo: ILinterInfo, resource?: Uri) { - const workspaceConfig = this.serviceContainer.get(IWorkspaceService); - const ws = workspaceConfig.getConfiguration('python.linting', resource); - const pe = ws!.inspect(linterInfo.enabledSettingName); - return (pe!.globalValue === undefined && pe!.workspaceValue === undefined && pe!.workspaceFolderValue === undefined); - } - public async getActiveLinters(silent: boolean, resource?: Uri): Promise { - if (silent) { - // only ask the user if they'd like to enable pylint when it is available... others may follow. - const pylintInfo = this.linters.find((linter: ILinterInfo) => linter.id === 'pylint'); - if (pylintInfo) { - await this.promptUserIfLinterAvailable(pylintInfo, resource); - } + if (!silent) { + await this.enableUnconfiguredLinters(resource); } return this.linters.filter(x => x.isEnabled(resource)); } public async setActiveLintersAsync(products: Product[], resource?: Uri): Promise { - const active = await this.getActiveLinters(false, resource); + const active = await this.getActiveLinters(true, resource); for (const x of active) { await x.enableAsync(false, resource); } @@ -211,7 +99,7 @@ export class LinterManager implements ILinterManager { } public async createLinter(product: Product, outputChannel: OutputChannel, serviceContainer: IServiceContainer, resource?: Uri): Promise { - if (!await this.isLintingEnabled(false, resource)) { + if (!await this.isLintingEnabled(true, resource)) { return new DisabledLinter(this.configService); } const error = 'Linter manager: Unknown linter'; @@ -238,4 +126,24 @@ export class LinterManager implements ILinterManager { } throw new Error(error); } + + private async enableUnconfiguredLinters(resource?: Uri): Promise { + // if we've already checked during this session, don't bother again + if (this.checkedForInstalledLinters) { + return false; + } + this.checkedForInstalledLinters = true; + + // only check & ask the user if they'd like to enable pylint + const pylintInfo = this.linters.find( + (linter: ILinterInfo) => linter.id === 'pylint' + ); + + // If linting is disabled, don't bother checking further. + if (pylintInfo && await this.isLintingEnabled(true, resource)) { + const activator = this.serviceContainer.get(IAvailableLinterActivator); + return activator.promptIfLinterAvailable(pylintInfo, resource); + } + return false; + } } diff --git a/src/client/linters/serviceRegistry.ts b/src/client/linters/serviceRegistry.ts index 89f7abb8e11f..0067340fe5ec 100644 --- a/src/client/linters/serviceRegistry.ts +++ b/src/client/linters/serviceRegistry.ts @@ -2,11 +2,15 @@ // Licensed under the MIT License. import { IServiceManager } from '../ioc/types'; +import { AvailableLinterActivator } from './linterAvailability'; import { LinterManager } from './linterManager'; import { LintingEngine } from './lintingEngine'; -import { ILinterManager, ILintingEngine } from './types'; +import { + IAvailableLinterActivator, ILinterManager, ILintingEngine +} from './types'; export function registerTypes(serviceManager: IServiceManager) { serviceManager.addSingleton(ILintingEngine, LintingEngine); serviceManager.addSingleton(ILinterManager, LinterManager); + serviceManager.add(IAvailableLinterActivator, AvailableLinterActivator); } diff --git a/src/client/linters/types.ts b/src/client/linters/types.ts index 4b0af07cc6e7..0c3e6f501d09 100644 --- a/src/client/linters/types.ts +++ b/src/client/linters/types.ts @@ -31,6 +31,11 @@ export interface ILinter { lint(document: vscode.TextDocument, cancellation: vscode.CancellationToken): Promise; } +export const IAvailableLinterActivator = Symbol('IAvailableLinterActivator'); +export interface IAvailableLinterActivator { + promptIfLinterAvailable(linter: ILinterInfo, resource?: vscode.Uri): Promise; +} + export const ILinterManager = Symbol('ILinterManager'); export interface ILinterManager { getAllLinterInfos(): ILinterInfo[]; From fd150e4f88ff4938c98864056f2bc9007df89d0d Mon Sep 17 00:00:00 2001 From: Derek Keeler Date: Fri, 19 Oct 2018 16:34:38 -0700 Subject: [PATCH 08/16] Rebase/merge problem resolved. --- src/client/linters/linterAvailability.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/linters/linterAvailability.ts b/src/client/linters/linterAvailability.ts index 80cce0dc8700..6c469e7cbd10 100644 --- a/src/client/linters/linterAvailability.ts +++ b/src/client/linters/linterAvailability.ts @@ -6,7 +6,7 @@ import { inject, injectable } from 'inversify'; import { Uri } from 'vscode'; import { IApplicationShell, IWorkspaceService } from '../common/application/types'; -import { error as traceError } from '../common/logger'; +import { traceError } from '../common/logger'; import { IInstaller, Product } from '../common/types'; import { IAvailableLinterActivator, ILinterInfo } from './types'; From 1f192c795b5a371d8b8dc02f146161fa2e631617 Mon Sep 17 00:00:00 2001 From: Derek Keeler Date: Mon, 22 Oct 2018 15:44:33 -0700 Subject: [PATCH 09/16] Tests for the linter manager --- src/client/linters/linterManager.ts | 2 +- src/test/linters/lint.manager.unit.test.ts | 101 +++++++++++++++++++++ 2 files changed, 102 insertions(+), 1 deletion(-) create mode 100644 src/test/linters/lint.manager.unit.test.ts diff --git a/src/client/linters/linterManager.ts b/src/client/linters/linterManager.ts index 719e2261daaa..67344c12407b 100644 --- a/src/client/linters/linterManager.ts +++ b/src/client/linters/linterManager.ts @@ -127,7 +127,7 @@ export class LinterManager implements ILinterManager { throw new Error(error); } - private async enableUnconfiguredLinters(resource?: Uri): Promise { + protected async enableUnconfiguredLinters(resource?: Uri): Promise { // if we've already checked during this session, don't bother again if (this.checkedForInstalledLinters) { return false; diff --git a/src/test/linters/lint.manager.unit.test.ts b/src/test/linters/lint.manager.unit.test.ts new file mode 100644 index 000000000000..66208a404628 --- /dev/null +++ b/src/test/linters/lint.manager.unit.test.ts @@ -0,0 +1,101 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +'use strict'; + +import { expect } from 'chai'; +import * as TypeMoq from 'typemoq'; +import { Uri } from 'vscode'; +import { IConfigurationService, IPythonSettings } from '../../client/common/types'; +import { IServiceContainer } from '../../client/ioc/types'; +import { LinterManager } from '../../client/linters/linterManager'; + +// setup class instance +class TestLinterManager extends LinterManager { + public enableUnconfiguredLintersCallCount: number = 0; + + protected async enableUnconfiguredLinters(resource?: Uri): Promise { + this.enableUnconfiguredLintersCallCount += 1; + return false; + } +} + +function getServiceContainerMockForLinterManagerTests(): TypeMoq.IMock { + // setup test mocks + const serviceContainerMock = TypeMoq.Mock.ofType(); + const configMock = TypeMoq.Mock.ofType(); + const pythonSettingsMock = TypeMoq.Mock.ofType(); + configMock.setup(cm => cm.getSettings(TypeMoq.It.isAny())).returns(() => pythonSettingsMock.object); + serviceContainerMock.setup(c => c.get(IConfigurationService)).returns(() => configMock.object); + + return serviceContainerMock; +} + +// tslint:disable-next-line:max-func-body-length +suite('Lint Manager Unit Tests', () => { + + test('Linter manager isLintingEnabled checks availability when silent = false.', async () => { + // set expectations + const expectedCallCount = 1; + const silentFlag = false; + + // get setup + const serviceContainerMock = getServiceContainerMockForLinterManagerTests(); + + // make the call + const lm = new TestLinterManager(serviceContainerMock.object); + await lm.isLintingEnabled(silentFlag); + + // test expectations + expect(lm.enableUnconfiguredLintersCallCount).to.equal(expectedCallCount); + }); + + test('Linter manager isLintingEnabled does not check availability when silent = true.', async () => { + // set expectations + const expectedCallCount = 0; + const silentFlag = true; + + // get setup + const serviceContainerMock = getServiceContainerMockForLinterManagerTests(); + + // make the call + const lm: TestLinterManager = new TestLinterManager(serviceContainerMock.object); + await lm.isLintingEnabled(silentFlag); + + // test expectations + expect(lm.enableUnconfiguredLintersCallCount).to.equal(expectedCallCount); + }); + + test('Linter manager getActiveLinters checks availability when silent = false.', async () => { + // set expectations + const expectedCallCount = 1; + const silentFlag = false; + + // get setup + const serviceContainerMock = getServiceContainerMockForLinterManagerTests(); + + // make the call + const lm: TestLinterManager = new TestLinterManager(serviceContainerMock.object); + await lm.getActiveLinters(silentFlag); + + // test expectations + expect(lm.enableUnconfiguredLintersCallCount).to.equal(expectedCallCount); + }); + + test('Linter manager getActiveLinters checks availability when silent = true.', async () => { + // set expectations + const expectedCallCount = 0; + const silentFlag = true; + + // get setup + const serviceContainerMock = getServiceContainerMockForLinterManagerTests(); + + // make the call + const lm: TestLinterManager = new TestLinterManager(serviceContainerMock.object); + await lm.getActiveLinters(silentFlag); + + // test expectations + expect(lm.enableUnconfiguredLintersCallCount).to.equal(expectedCallCount); + }); + +}); From 3bac1b78e28b3b27106af3f4c53ee48803e120e0 Mon Sep 17 00:00:00 2001 From: Derek Keeler Date: Mon, 22 Oct 2018 19:32:39 -0700 Subject: [PATCH 10/16] Add tests for Linter Availability - add check for 'is feature enabled' and test for it too - feature is enabled when python.jediEnabled=false by default --- src/client/linters/linterAvailability.ts | 21 +- .../linters/linter.availability.unit.test.ts | 604 ++++++++++++++++++ 2 files changed, 624 insertions(+), 1 deletion(-) create mode 100644 src/test/linters/linter.availability.unit.test.ts diff --git a/src/client/linters/linterAvailability.ts b/src/client/linters/linterAvailability.ts index 6c469e7cbd10..4de0e1199444 100644 --- a/src/client/linters/linterAvailability.ts +++ b/src/client/linters/linterAvailability.ts @@ -29,6 +29,10 @@ export class AvailableLinterActivator implements IAvailableLinterActivator { * @returns true if configuration was updated in any way, false otherwise. */ public async promptIfLinterAvailable(linterInfo: ILinterInfo, resource?: Uri): Promise { + // Has the feature been enabled yet? + if (!this.isFeatureEnabled()) { + return false; + } // Has the linter in question has been configured explicitly? If so, no need to continue. if (!this.isLinterUsingDefaultConfiguration(linterInfo, resource)) { @@ -101,9 +105,24 @@ export class AvailableLinterActivator implements IAvailableLinterActivator { * * @returns true if the linter has not been configured at the user, workspace, or workspace-folder scope. false otherwise. */ - public isLinterUsingDefaultConfiguration(linterInfo: ILinterInfo, resource?: Uri) { + public isLinterUsingDefaultConfiguration(linterInfo: ILinterInfo, resource?: Uri): boolean { const ws = this.workspaceConfig.getConfiguration('python.linting', resource); const pe = ws!.inspect(linterInfo.enabledSettingName); return (pe!.globalValue === undefined && pe!.workspaceValue === undefined && pe!.workspaceFolderValue === undefined); } + + /** + * Check if this feature is enabled yet. + * + * This is a feature of the vscode-python extension that will become enabled once the + * Python Language Server becomes the default, replacing Jedi as the default. Testing + * the global default setting for `"python.jediEnabled": false` enables it. + * + * @returns true if the global default for python.jediEnabled is false. + */ + public isFeatureEnabled(): boolean { + const ws = this.workspaceConfig.getConfiguration('python'); + const jediEnabled = ws!.inspect('jediEnabled'); + return (!jediEnabled || jediEnabled.defaultValue === false); + } } diff --git a/src/test/linters/linter.availability.unit.test.ts b/src/test/linters/linter.availability.unit.test.ts new file mode 100644 index 000000000000..a8dad0bd2735 --- /dev/null +++ b/src/test/linters/linter.availability.unit.test.ts @@ -0,0 +1,604 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +'use strict'; + +import { expect } from 'chai'; +import * as TypeMoq from 'typemoq'; +import { Uri, WorkspaceConfiguration } from 'vscode'; +import { IApplicationShell, IWorkspaceService } from '../../client/common/application/types'; +import { IConfigurationService, IInstaller, Product } from '../../client/common/types'; +import { AvailableLinterActivator } from '../../client/linters/linterAvailability'; +import { LinterInfo } from '../../client/linters/linterInfo'; +import { IAvailableLinterActivator } from '../../client/linters/types'; + +// tslint:disable-next-line:max-func-body-length +suite('Linter Availability Provider tests', () => { + + test('Availability feature is disabled when global default for jediEnabled=true.', async () => { + // set expectations + const jediEnabledValue = true; + const expectedResult = false; + + // arrange + const appShellMock = TypeMoq.Mock.ofType(); + const installerMock = TypeMoq.Mock.ofType(); + const wkspcConfigSrvcMock = setupWorkspaceServiceForJediSettingTest(jediEnabledValue); + + // call + const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); + const result = availabilityProvider.isFeatureEnabled(); + + // check expectaions + expect(result).is.equal(expectedResult); + wkspcConfigSrvcMock.verifyAll(); + }); + + test('Availability feature is enabled when global default for jediEnabled=false.', async () => { + // set expectations + const jediEnabledValue = false; + const expectedResult = true; + + // arrange + const appShellMock = TypeMoq.Mock.ofType(); + const installerMock = TypeMoq.Mock.ofType(); + const wkspcConfigSrvcMock = setupWorkspaceServiceForJediSettingTest(jediEnabledValue); + + const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); + + const result = availabilityProvider.isFeatureEnabled(); + + expect(result).is.equal(expectedResult); + wkspcConfigSrvcMock.verifyAll(); + }); + + test('Prompt will be performed when linter is not configured at all for the workspace, workspace-folder, or the user', async () => { + // setup expectations + const pylintUserValue = undefined; + const pylintWorkspaceValue = undefined; + const pylintWorkspaceFolderValue = undefined; + const expectedResult = true; + + const appShellMock = TypeMoq.Mock.ofType(); + const installerMock = TypeMoq.Mock.ofType(); + const configServiceMock = TypeMoq.Mock.ofType(); + const wkspcConfigSrvcMock = setupWorkspaceMockForLinterConfiguredTests(pylintUserValue, pylintWorkspaceValue, pylintWorkspaceFolderValue); + + const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); + + const pylintInfo = new LinterInfo(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']); + + const result = availabilityProvider.isLinterUsingDefaultConfiguration(pylintInfo); + + expect(result).to.equal(expectedResult); + wkspcConfigSrvcMock.verifyAll(); + }); + + test('No prompt performed when linter is configured as enabled for the workspace', async () => { + // setup expectations + const pylintUserValue = undefined; + const pylintWorkspaceValue = true; + const pylintWorkspaceFolderValue = undefined; + const expectedResult = false; + + const appShellMock = TypeMoq.Mock.ofType(); + const installerMock = TypeMoq.Mock.ofType(); + const configServiceMock = TypeMoq.Mock.ofType(); + const wkspcConfigSrvcMock = setupWorkspaceMockForLinterConfiguredTests(pylintUserValue, pylintWorkspaceValue, pylintWorkspaceFolderValue); + + const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); + + const pylintInfo = new LinterInfo(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']); + const result = availabilityProvider.isLinterUsingDefaultConfiguration(pylintInfo); + expect(result).to.equal(expectedResult); + wkspcConfigSrvcMock.verifyAll(); + }); + + test('No prompt performed when linter is configured as enabled for the entire user', async () => { + // setup expectations + const pylintUserValue = true; + const pylintWorkspaceValue = undefined; + const pylintWorkspaceFolderValue = undefined; + const expectedResult = false; + + const appShellMock = TypeMoq.Mock.ofType(); + const installerMock = TypeMoq.Mock.ofType(); + const configServiceMock = TypeMoq.Mock.ofType(); + const wkspcConfigSrvcMock = setupWorkspaceMockForLinterConfiguredTests(pylintUserValue, pylintWorkspaceValue, pylintWorkspaceFolderValue); + + const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); + + const pylintInfo = new LinterInfo(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']); + const result = availabilityProvider.isLinterUsingDefaultConfiguration(pylintInfo); + expect(result).to.equal(expectedResult); + wkspcConfigSrvcMock.verifyAll(); + }); + + test('No prompt performed when linter is configured as enabled for the workspace-folder', async () => { + // setup expectations + const pylintUserValue = undefined; + const pylintWorkspaceValue = undefined; + const pylintWorkspaceFolderValue = true; + const expectedResult = false; + + const appShellMock = TypeMoq.Mock.ofType(); + const installerMock = TypeMoq.Mock.ofType(); + const configServiceMock = TypeMoq.Mock.ofType(); + + const wkspcConfigSrvcMock = setupWorkspaceMockForLinterConfiguredTests(pylintUserValue, pylintWorkspaceValue, pylintWorkspaceFolderValue); + + const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); + + const pylintInfo = new LinterInfo(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']); + const result = availabilityProvider.isLinterUsingDefaultConfiguration(pylintInfo); + expect(result).to.equal(expectedResult); + wkspcConfigSrvcMock.verifyAll(); + }); + + test('Linter is enabled after being prompted and "Enable " is selected', async () => { + // set expectation + const promptReply = { title: 'Enable pylint', enabled: true }; + const expectedResult = true; + + // arrange + const appShellMock = TypeMoq.Mock.ofType(); + const installerMock = TypeMoq.Mock.ofType(); + const wkspcConfigSrvcMock = TypeMoq.Mock.ofType(); + + const configServiceMock = TypeMoq.Mock.ofType(); + + const linterInfo = new class extends LinterInfo { + public testIsEnabled: boolean = !promptReply.enabled; + + public async enableAsync(enabled: boolean, resource?: Uri): Promise { + this.testIsEnabled = enabled; + return Promise.resolve(); + } + + }(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']); + + appShellMock.setup(ap => ap.showInformationMessage( + TypeMoq.It.isValue(`Linter ${linterInfo.id} is available but not enabled.`), + TypeMoq.It.isAny(), + TypeMoq.It.isAny()) + ) + .returns(() => { + // tslint:disable-next-line:no-any + return promptReply as any; + }) + .verifiable(TypeMoq.Times.once()); + + // call + const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); + const result = await availabilityProvider.promptToConfigureAvailableLinter(linterInfo); + + expect(result).to.equal(expectedResult, 'Expected promptToConfigureAvailableLinter to return true because the configuration was updated.'); + expect(linterInfo.testIsEnabled).to.equal(promptReply.enabled, 'LinterInfo test class was not updated as a result of the test.'); + }); + + test('Linter is disabled after being prompted and "Disable " is selected', async () => { + // set expectation + const promptReply = { title: 'Disable pylint', enabled: false }; + const expectedResult = true; + + // arrange + const appShellMock = TypeMoq.Mock.ofType(); + const installerMock = TypeMoq.Mock.ofType(); + const wkspcConfigSrvcMock = TypeMoq.Mock.ofType(); + + const configServiceMock = TypeMoq.Mock.ofType(); + + const linterInfo = new class extends LinterInfo { + public testIsEnabled: boolean = !promptReply.enabled; + + public async enableAsync(enabled: boolean, resource?: Uri): Promise { + this.testIsEnabled = enabled; + return Promise.resolve(); + } + + }(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']); + + appShellMock.setup(ap => ap.showInformationMessage( + TypeMoq.It.isValue(`Linter ${linterInfo.id} is available but not enabled.`), + TypeMoq.It.isAny(), + TypeMoq.It.isAny()) + ) + .returns(() => { + // tslint:disable-next-line:no-any + return promptReply as any; + }) + .verifiable(TypeMoq.Times.once()); + + // call + const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); + const result = await availabilityProvider.promptToConfigureAvailableLinter(linterInfo); + + expect(result).to.equal(expectedResult, 'Expected promptToConfigureAvailableLinter to return true because the configuration was updated.'); + expect(linterInfo.testIsEnabled).to.equal(promptReply.enabled, 'LinterInfo test class was not updated as a result of the test.'); + }); + + test('Linter is left unconfigured after being prompted and the prompt is disabled without any selection made', async () => { + // set expectation + const promptReply = undefined; + const expectedResult = false; + + // arrange + const appShellMock = TypeMoq.Mock.ofType(); + const installerMock = TypeMoq.Mock.ofType(); + const wkspcConfigSrvcMock = TypeMoq.Mock.ofType(); + + const configServiceMock = TypeMoq.Mock.ofType(); + + const linterInfo = new class extends LinterInfo { + public testIsEnabled: boolean = false; + + public async enableAsync(enabled: boolean, resource?: Uri): Promise { + this.testIsEnabled = enabled; + return Promise.resolve(); + } + + }(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']); + + appShellMock.setup(ap => ap.showInformationMessage( + TypeMoq.It.isValue(`Linter ${linterInfo.id} is available but not enabled.`), + TypeMoq.It.isAny(), + TypeMoq.It.isAny()) + ) + .returns(() => { + // tslint:disable-next-line:no-any + return promptReply as any; + }) + .verifiable(TypeMoq.Times.once()); + + // call + const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); + const result = await availabilityProvider.promptToConfigureAvailableLinter(linterInfo); + + expect(result).to.equal(expectedResult, 'Expected promptToConfigureAvailableLinter to return true because the configuration was updated.'); + }); + + test('Overall implementation does not change configuration when feature disabled', async () => { + // set expectations + const jediEnabledValue = true; + const expectedResult = false; + + // arrange + const appShellMock = TypeMoq.Mock.ofType(); + const installerMock = TypeMoq.Mock.ofType(); + const configServiceMock = TypeMoq.Mock.ofType(); + const wkspcConfigSrvcMock = setupWorkspaceServiceForJediSettingTest(jediEnabledValue); + const pylintInfo = new LinterInfo(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']); + + // perform test + const availabilityProvider: IAvailableLinterActivator = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); + const result = await availabilityProvider.promptIfLinterAvailable(pylintInfo); + + expect(expectedResult).to.equal(result, 'promptIfLinterAvailable should not change any configuration when python.jediEnabled is true.'); + wkspcConfigSrvcMock.verifyAll(); + }); + + test('Overall implementation does not change configuration when linter is configured (enabled)', async () => { + // set expectations + const jediEnabledValue = false; + const pylintUserEnabled = undefined; + const pylintWorkspaceEnabled = true; + const pylintWorkspaceFolderEnabled = undefined; + const expectedResult = false; + + // arrange + const appShellMock = TypeMoq.Mock.ofType(); + const installerMock = TypeMoq.Mock.ofType(); + const configServiceMock = TypeMoq.Mock.ofType(); + let wkspcConfigSrvcMock = setupWorkspaceServiceForJediSettingTest(jediEnabledValue); + wkspcConfigSrvcMock = setupWorkspaceMockForLinterConfiguredTests(pylintUserEnabled, pylintWorkspaceEnabled, pylintWorkspaceFolderEnabled, wkspcConfigSrvcMock); + const pylintInfo = new LinterInfo(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']); + + // perform test + const availabilityProvider: IAvailableLinterActivator = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); + const result = await availabilityProvider.promptIfLinterAvailable(pylintInfo); + + // perform test + expect(expectedResult).to.equal(result, 'Configuration should not change if the linter is configured in any way.'); + wkspcConfigSrvcMock.verifyAll(); + }); + + test('Overall implementation does not change configuration when linter is configured (disabled)', async () => { + // set expectations + const jediEnabledValue = false; + const pylintUserEnabled = undefined; + const pylintWorkspaceEnabled = true; + const pylintWorkspaceFolderEnabled = undefined; + const expectedResult = false; + + // arrange + const appShellMock = TypeMoq.Mock.ofType(); + const installerMock = TypeMoq.Mock.ofType(); + const configServiceMock = TypeMoq.Mock.ofType(); + let wkspcConfigSrvcMock = setupWorkspaceServiceForJediSettingTest(jediEnabledValue); + wkspcConfigSrvcMock = setupWorkspaceMockForLinterConfiguredTests(pylintUserEnabled, pylintWorkspaceEnabled, pylintWorkspaceFolderEnabled, wkspcConfigSrvcMock); + const pylintInfo = new LinterInfo(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']); + + // perform test + const availabilityProvider: IAvailableLinterActivator = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); + const result = await availabilityProvider.promptIfLinterAvailable(pylintInfo); + + // perform test + expect(expectedResult).to.equal(result, 'Configuration should not change if the linter is disabled in any way.'); + wkspcConfigSrvcMock.verifyAll(); + }); + + test('Overall implementation does not change configuration when linter is unavailable in current workspace environment', async () => { + // set expectations + const jediEnabledValue = false; + const pylintUserEnabled = undefined; + const pylintWorkspaceEnabled = true; + const pylintWorkspaceFolderEnabled = undefined; + const linterIsInstalled = false; + const expectedResult = false; + + // arrange + const appShellMock = TypeMoq.Mock.ofType(); + const configServiceMock = TypeMoq.Mock.ofType(); + const pylintInfo = new LinterInfo(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']); + const installerMock = TypeMoq.Mock.ofType(); + installerMock.setup(im => im.isInstalled(pylintInfo.product, TypeMoq.It.isAny())) + .returns(async () => linterIsInstalled) + .verifiable(TypeMoq.Times.once()); + let wkspcConfigSrvcMock = setupWorkspaceServiceForJediSettingTest(jediEnabledValue); + wkspcConfigSrvcMock = setupWorkspaceMockForLinterConfiguredTests(pylintUserEnabled, pylintWorkspaceEnabled, pylintWorkspaceFolderEnabled, wkspcConfigSrvcMock); + + // perform test + const availabilityProvider: IAvailableLinterActivator = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); + const result = await availabilityProvider.promptIfLinterAvailable(pylintInfo); + + expect(expectedResult).to.equal(result, 'Configuration should not change if the linter is unavailable in the current workspace environment.'); + wkspcConfigSrvcMock.verifyAll(); + }); + + test('Overall implementation does not change configuration when user is prompted and prompt is dismissed', async () => { + // set expectations + const jediEnabledValue = false; + const pylintUserEnabled = undefined; + const pylintWorkspaceEnabled = undefined; + const pylintWorkspaceFolderEnabled = undefined; + const linterIsInstalled = true; + const promptReply = undefined; + const expectedResult = false; + + // arrange + const configServiceMock = TypeMoq.Mock.ofType(); + const pylintInfo = new LinterInfo(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']); + const appShellMock = TypeMoq.Mock.ofType(); + appShellMock.setup(ap => ap.showInformationMessage( + TypeMoq.It.isValue(`Linter ${pylintInfo.id} is available but not enabled.`), + TypeMoq.It.isAny(), + TypeMoq.It.isAny()) + ) + // tslint:disable-next-line:no-any + .returns(() => promptReply as any) + .verifiable(TypeMoq.Times.once()); + const installerMock = TypeMoq.Mock.ofType(); + installerMock.setup(im => im.isInstalled(pylintInfo.product, TypeMoq.It.isAny())) + .returns(async () => linterIsInstalled) + .verifiable(TypeMoq.Times.once()); + let wkspcConfigSrvcMock = setupWorkspaceServiceForJediSettingTest(jediEnabledValue); + wkspcConfigSrvcMock = setupWorkspaceMockForLinterConfiguredTests(pylintUserEnabled, pylintWorkspaceEnabled, pylintWorkspaceFolderEnabled, wkspcConfigSrvcMock); + + // perform test + const availabilityProvider: IAvailableLinterActivator = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); + const result = await availabilityProvider.promptIfLinterAvailable(pylintInfo); + + expect(expectedResult).to.equal(result, 'Configuration should not change if the user is prompted and they dismiss the prompt.'); + wkspcConfigSrvcMock.verifyAll(); + appShellMock.verifyAll(); + installerMock.verifyAll(); + }); + + test('Overall implementation changes configuration when user is prompted and "Disable " is selected', async () => { + // set expectations + const jediEnabledValue = false; + const pylintUserEnabled = undefined; + const pylintWorkspaceEnabled = undefined; + const pylintWorkspaceFolderEnabled = undefined; + const linterIsInstalled = true; + const promptReply = { title: 'Disable pylint', enabled: false }; + const expectedResult = true; + + // arrange + const configServiceMock = TypeMoq.Mock.ofType(); + const pylintInfo = new LinterInfo(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']); + const appShellMock = TypeMoq.Mock.ofType(); + appShellMock.setup(ap => ap.showInformationMessage( + TypeMoq.It.isValue(`Linter ${pylintInfo.id} is available but not enabled.`), + TypeMoq.It.isAny(), + TypeMoq.It.isAny()) + ) + .returns(() => { + // tslint:disable-next-line:no-any + return promptReply as any; + }) + .verifiable(TypeMoq.Times.once()); + const installerMock = TypeMoq.Mock.ofType(); + installerMock.setup(im => im.isInstalled(pylintInfo.product, TypeMoq.It.isAny())) + .returns(async () => linterIsInstalled) + .verifiable(TypeMoq.Times.once()); + let wkspcConfigSrvcMock = setupWorkspaceServiceForJediSettingTest(jediEnabledValue); + wkspcConfigSrvcMock = setupWorkspaceMockForLinterConfiguredTests(pylintUserEnabled, pylintWorkspaceEnabled, pylintWorkspaceFolderEnabled, wkspcConfigSrvcMock); + + // perform test + const availabilityProvider: IAvailableLinterActivator = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); + const result = await availabilityProvider.promptIfLinterAvailable(pylintInfo); + + expect(expectedResult).to.equal(result, 'Configuration should not change if the user is prompted and they dismiss the prompt.'); + wkspcConfigSrvcMock.verifyAll(); + appShellMock.verifyAll(); + installerMock.verifyAll(); + }); + + test('Overall implementation changes configuration when user is prompted and "Enable " is selected', async () => { + // set expectations + const jediEnabledValue = false; + const pylintUserEnabled = undefined; + const pylintWorkspaceEnabled = undefined; + const pylintWorkspaceFolderEnabled = undefined; + const linterIsInstalled = true; + const promptReply = { title: 'Enable pylint', enabled: true }; + const expectedResult = true; + + // arrange + const configServiceMock = TypeMoq.Mock.ofType(); + const pylintInfo = new LinterInfo(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']); + const appShellMock = TypeMoq.Mock.ofType(); + appShellMock.setup(ap => ap.showInformationMessage( + TypeMoq.It.isValue(`Linter ${pylintInfo.id} is available but not enabled.`), + TypeMoq.It.isAny(), + TypeMoq.It.isAny()) + ) + .returns(() => { + // tslint:disable-next-line:no-any + return promptReply as any; + }) + .verifiable(TypeMoq.Times.once()); + const installerMock = TypeMoq.Mock.ofType(); + installerMock.setup(im => im.isInstalled(pylintInfo.product, TypeMoq.It.isAny())) + .returns(async () => linterIsInstalled) + .verifiable(TypeMoq.Times.once()); + let wkspcConfigSrvcMock = setupWorkspaceServiceForJediSettingTest(jediEnabledValue); + wkspcConfigSrvcMock = setupWorkspaceMockForLinterConfiguredTests(pylintUserEnabled, pylintWorkspaceEnabled, pylintWorkspaceFolderEnabled, wkspcConfigSrvcMock); + + // perform test + const availabilityProvider: IAvailableLinterActivator = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); + const result = await availabilityProvider.promptIfLinterAvailable(pylintInfo); + + expect(expectedResult).to.equal(result, 'Configuration should not change if the user is prompted and they dismiss the prompt.'); + wkspcConfigSrvcMock.verifyAll(); + appShellMock.verifyAll(); + installerMock.verifyAll(); + }); + + test('Discovery of linter is available in the environment returns true when it succeeds and is present', async () => { + // set expectations + const linterIsInstalled = true; + const expectedResult = true; + + // arrange + const appShellMock = TypeMoq.Mock.ofType(); + const installerMock = TypeMoq.Mock.ofType(); + const configServiceMock = TypeMoq.Mock.ofType(); + const wkspcConfigSrvcMock = TypeMoq.Mock.ofType(); + + const pylintInfo = new LinterInfo(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']); + + installerMock.setup(im => im.isInstalled(pylintInfo.product, TypeMoq.It.isAny())) + .returns(async () => linterIsInstalled) + .verifiable(TypeMoq.Times.once()); + + // perform test + const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); + const result = await availabilityProvider.isLinterAvailable(pylintInfo.product); + + expect(result).to.equal(expectedResult, 'Expected promptToConfigureAvailableLinter to return true because the configuration was updated.'); + installerMock.verifyAll(); + }); + + test('Discovery of linter is available in the environment returns false when it succeeds and is not present', async () => { + // set expectations + const linterIsInstalled = false; + const expectedResult = false; + + // arrange + const appShellMock = TypeMoq.Mock.ofType(); + const installerMock = TypeMoq.Mock.ofType(); + const configServiceMock = TypeMoq.Mock.ofType(); + const wkspcConfigSrvcMock = TypeMoq.Mock.ofType(); + + const pylintInfo = new LinterInfo(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']); + + installerMock.setup(im => im.isInstalled(pylintInfo.product, TypeMoq.It.isAny())) + .returns(async () => linterIsInstalled) + .verifiable(TypeMoq.Times.once()); + + // perform test + const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); + const result = await availabilityProvider.isLinterAvailable(pylintInfo.product); + + expect(result).to.equal(expectedResult, 'Expected promptToConfigureAvailableLinter to return true because the configuration was updated.'); + installerMock.verifyAll(); + }); + + test('Discovery of linter is available in the environment returns false when it fails', async () => { + // set expectations + const expectedResult = false; + + // arrange + const appShellMock = TypeMoq.Mock.ofType(); + const installerMock = TypeMoq.Mock.ofType(); + const configServiceMock = TypeMoq.Mock.ofType(); + const wkspcConfigSrvcMock = TypeMoq.Mock.ofType(); + + const pylintInfo = new LinterInfo(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']); + + installerMock.setup(im => im.isInstalled(pylintInfo.product, TypeMoq.It.isAny())) + .returns(() => Promise.reject('error testfail')) + .verifiable(TypeMoq.Times.once()); + + // perform test + const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); + const result = await availabilityProvider.isLinterAvailable(pylintInfo.product); + + expect(result).to.equal(expectedResult, 'Expected promptToConfigureAvailableLinter to return true because the configuration was updated.'); + installerMock.verifyAll(); + }); +}); + +function setupWorkspaceMockForLinterConfiguredTests( + enabledForUser: boolean | undefined, + enabeldForWorkspace: boolean | undefined, + enabledForWorkspaceFolder: boolean | undefined, + wkspcConfigSrvcMock?: TypeMoq.IMock): TypeMoq.IMock { + + if (!wkspcConfigSrvcMock) { + wkspcConfigSrvcMock = TypeMoq.Mock.ofType(); + } + const workspaceConfiguration = TypeMoq.Mock.ofType(); + workspaceConfiguration.setup(wc => wc.inspect(TypeMoq.It.isValue('pylintEnabled'))) + .returns(() => { + return { + key: '', + globalValue: enabledForUser, + defaultValue: false, + workspaceFolderValue: enabeldForWorkspace, + workspaceValue: enabledForWorkspaceFolder + }; + }) + .verifiable(TypeMoq.Times.once()); + + wkspcConfigSrvcMock.setup(ws => ws.getConfiguration(TypeMoq.It.isValue('python.linting'), TypeMoq.It.isAny())) + .returns(() => workspaceConfiguration.object) + .verifiable(TypeMoq.Times.once()); + + return wkspcConfigSrvcMock; +} + +function setupWorkspaceServiceForJediSettingTest(jediEnabledValue: boolean, wkspcConfigSrvcMock?: TypeMoq.IMock): TypeMoq.IMock { + + if (!wkspcConfigSrvcMock) { + wkspcConfigSrvcMock = TypeMoq.Mock.ofType(); + } + const workspaceConfiguration = TypeMoq.Mock.ofType(); + workspaceConfiguration.setup(wc => wc.inspect(TypeMoq.It.isValue('jediEnabled'))) + .returns(() => { + return { + key: '', + globalValue: undefined, + defaultValue: jediEnabledValue, + workspaceFolderValue: undefined, + workspaceValue: undefined + }; + }) + .verifiable(TypeMoq.Times.once()); + wkspcConfigSrvcMock.setup(ws => ws.getConfiguration(TypeMoq.It.isValue('python'))) + .returns(() => workspaceConfiguration.object) + .verifiable(TypeMoq.Times.once()); + return wkspcConfigSrvcMock; +} From 7d0919cf7309b4df23a2ddffdad2b46eeada2195 Mon Sep 17 00:00:00 2001 From: Derek Keeler Date: Tue, 23 Oct 2018 13:16:29 -0700 Subject: [PATCH 11/16] PR suggestions - Also fixed up tests to make them far simpler to read. --- src/client/linters/linterAvailability.ts | 4 +- .../linters/linter.availability.unit.test.ts | 385 ++++++------------ 2 files changed, 121 insertions(+), 268 deletions(-) diff --git a/src/client/linters/linterAvailability.ts b/src/client/linters/linterAvailability.ts index 4de0e1199444..692be9cccc2f 100644 --- a/src/client/linters/linterAvailability.ts +++ b/src/client/linters/linterAvailability.ts @@ -30,7 +30,7 @@ export class AvailableLinterActivator implements IAvailableLinterActivator { */ public async promptIfLinterAvailable(linterInfo: ILinterInfo, resource?: Uri): Promise { // Has the feature been enabled yet? - if (!this.isFeatureEnabled()) { + if (!this.isFeatureEnabled) { return false; } @@ -120,7 +120,7 @@ export class AvailableLinterActivator implements IAvailableLinterActivator { * * @returns true if the global default for python.jediEnabled is false. */ - public isFeatureEnabled(): boolean { + public get isFeatureEnabled(): boolean { const ws = this.workspaceConfig.getConfiguration('python'); const jediEnabled = ws!.inspect('jediEnabled'); return (!jediEnabled || jediEnabled.defaultValue === false); diff --git a/src/test/linters/linter.availability.unit.test.ts b/src/test/linters/linter.availability.unit.test.ts index a8dad0bd2735..9d761cfe2cf5 100644 --- a/src/test/linters/linter.availability.unit.test.ts +++ b/src/test/linters/linter.availability.unit.test.ts @@ -21,16 +21,14 @@ suite('Linter Availability Provider tests', () => { const expectedResult = false; // arrange - const appShellMock = TypeMoq.Mock.ofType(); - const installerMock = TypeMoq.Mock.ofType(); - const wkspcConfigSrvcMock = setupWorkspaceServiceForJediSettingTest(jediEnabledValue); + const [appShellMock, installerMock, wkspcConfigSrvcMock] = getDependenciesForAvailabilityTests(); + setupWorkspaceServiceForJediSettingTest(jediEnabledValue, wkspcConfigSrvcMock); // call const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); - const result = availabilityProvider.isFeatureEnabled(); // check expectaions - expect(result).is.equal(expectedResult); + expect(availabilityProvider.isFeatureEnabled).is.equal(expectedResult, 'Avaialability feature should be disabled when python.jediEnabled defaults to true'); wkspcConfigSrvcMock.verifyAll(); }); @@ -40,15 +38,12 @@ suite('Linter Availability Provider tests', () => { const expectedResult = true; // arrange - const appShellMock = TypeMoq.Mock.ofType(); - const installerMock = TypeMoq.Mock.ofType(); - const wkspcConfigSrvcMock = setupWorkspaceServiceForJediSettingTest(jediEnabledValue); + const [appShellMock, installerMock, wkspcConfigSrvcMock] = getDependenciesForAvailabilityTests(); + setupWorkspaceServiceForJediSettingTest(jediEnabledValue, wkspcConfigSrvcMock); const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); - const result = availabilityProvider.isFeatureEnabled(); - - expect(result).is.equal(expectedResult); + expect(availabilityProvider.isFeatureEnabled).is.equal(expectedResult, 'Avaialability feature should be enabled when python.jediEnabled defaults to false'); wkspcConfigSrvcMock.verifyAll(); }); @@ -59,18 +54,14 @@ suite('Linter Availability Provider tests', () => { const pylintWorkspaceFolderValue = undefined; const expectedResult = true; - const appShellMock = TypeMoq.Mock.ofType(); - const installerMock = TypeMoq.Mock.ofType(); - const configServiceMock = TypeMoq.Mock.ofType(); - const wkspcConfigSrvcMock = setupWorkspaceMockForLinterConfiguredTests(pylintUserValue, pylintWorkspaceValue, pylintWorkspaceFolderValue); + const [appShellMock, installerMock, wkspcConfigSrvcMock, linterInfo] = getDependenciesForAvailabilityTests(); + setupWorkspaceMockForLinterConfiguredTests(pylintUserValue, pylintWorkspaceValue, pylintWorkspaceFolderValue, wkspcConfigSrvcMock); const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); - const pylintInfo = new LinterInfo(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']); - - const result = availabilityProvider.isLinterUsingDefaultConfiguration(pylintInfo); + const result = availabilityProvider.isLinterUsingDefaultConfiguration(linterInfo); - expect(result).to.equal(expectedResult); + expect(result).to.equal(expectedResult, 'Linter is unconfigured but prompt did not get raised'); wkspcConfigSrvcMock.verifyAll(); }); @@ -81,16 +72,13 @@ suite('Linter Availability Provider tests', () => { const pylintWorkspaceFolderValue = undefined; const expectedResult = false; - const appShellMock = TypeMoq.Mock.ofType(); - const installerMock = TypeMoq.Mock.ofType(); - const configServiceMock = TypeMoq.Mock.ofType(); - const wkspcConfigSrvcMock = setupWorkspaceMockForLinterConfiguredTests(pylintUserValue, pylintWorkspaceValue, pylintWorkspaceFolderValue); + const [appShellMock, installerMock, wkspcConfigSrvcMock, linterInfo] = getDependenciesForAvailabilityTests(); + setupWorkspaceMockForLinterConfiguredTests(pylintUserValue, pylintWorkspaceValue, pylintWorkspaceFolderValue, wkspcConfigSrvcMock); const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); - const pylintInfo = new LinterInfo(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']); - const result = availabilityProvider.isLinterUsingDefaultConfiguration(pylintInfo); - expect(result).to.equal(expectedResult); + const result = availabilityProvider.isLinterUsingDefaultConfiguration(linterInfo); + expect(result).to.equal(expectedResult, 'Available linter prompt should not be shown when linter is configured for workspace.'); wkspcConfigSrvcMock.verifyAll(); }); @@ -101,16 +89,13 @@ suite('Linter Availability Provider tests', () => { const pylintWorkspaceFolderValue = undefined; const expectedResult = false; - const appShellMock = TypeMoq.Mock.ofType(); - const installerMock = TypeMoq.Mock.ofType(); - const configServiceMock = TypeMoq.Mock.ofType(); - const wkspcConfigSrvcMock = setupWorkspaceMockForLinterConfiguredTests(pylintUserValue, pylintWorkspaceValue, pylintWorkspaceFolderValue); - + // arrange + const [appShellMock, installerMock, wkspcConfigSrvcMock, linterInfo] = getDependenciesForAvailabilityTests(); + setupWorkspaceMockForLinterConfiguredTests(pylintUserValue, pylintWorkspaceValue, pylintWorkspaceFolderValue, wkspcConfigSrvcMock); const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); - const pylintInfo = new LinterInfo(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']); - const result = availabilityProvider.isLinterUsingDefaultConfiguration(pylintInfo); - expect(result).to.equal(expectedResult); + const result = availabilityProvider.isLinterUsingDefaultConfiguration(linterInfo); + expect(result).to.equal(expectedResult, 'Available linter prompt should not be shown when linter is configured for user.'); wkspcConfigSrvcMock.verifyAll(); }); @@ -121,34 +106,23 @@ suite('Linter Availability Provider tests', () => { const pylintWorkspaceFolderValue = true; const expectedResult = false; - const appShellMock = TypeMoq.Mock.ofType(); - const installerMock = TypeMoq.Mock.ofType(); - const configServiceMock = TypeMoq.Mock.ofType(); - - const wkspcConfigSrvcMock = setupWorkspaceMockForLinterConfiguredTests(pylintUserValue, pylintWorkspaceValue, pylintWorkspaceFolderValue); - + // arrange + const [appShellMock, installerMock, wkspcConfigSrvcMock, linterInfo] = getDependenciesForAvailabilityTests(); + setupWorkspaceMockForLinterConfiguredTests(pylintUserValue, pylintWorkspaceValue, pylintWorkspaceFolderValue, wkspcConfigSrvcMock); const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); - const pylintInfo = new LinterInfo(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']); - const result = availabilityProvider.isLinterUsingDefaultConfiguration(pylintInfo); - expect(result).to.equal(expectedResult); + const result = availabilityProvider.isLinterUsingDefaultConfiguration(linterInfo); + expect(result).to.equal(expectedResult, 'Available linter prompt should not be shown when linter is configured for workspace-folder.'); wkspcConfigSrvcMock.verifyAll(); }); - test('Linter is enabled after being prompted and "Enable " is selected', async () => { - // set expectation - const promptReply = { title: 'Enable pylint', enabled: true }; - const expectedResult = true; - + async function testForLinterPromptResponse(promptReply: { title: string; enabled: boolean } | undefined): Promise { // arrange - const appShellMock = TypeMoq.Mock.ofType(); - const installerMock = TypeMoq.Mock.ofType(); - const wkspcConfigSrvcMock = TypeMoq.Mock.ofType(); - + const [appShellMock, installerMock, wkspcConfigSrvcMock] = getDependenciesForAvailabilityTests(); const configServiceMock = TypeMoq.Mock.ofType(); const linterInfo = new class extends LinterInfo { - public testIsEnabled: boolean = !promptReply.enabled; + public testIsEnabled: boolean = promptReply ? promptReply.enabled : false; public async enableAsync(enabled: boolean, resource?: Uri): Promise { this.testIsEnabled = enabled; @@ -168,12 +142,26 @@ suite('Linter Availability Provider tests', () => { }) .verifiable(TypeMoq.Times.once()); - // call + // perform test const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); const result = await availabilityProvider.promptToConfigureAvailableLinter(linterInfo); + if (promptReply) { + expect(linterInfo.testIsEnabled).to.equal(promptReply.enabled, 'LinterInfo test class was not updated as a result of the test.'); + } + + return result; + } + + test('Linter is enabled after being prompted and "Enable " is selected', async () => { + // set expectations + const expectedResult = true; + const promptReply = { title: 'Enable pylint', enabled: true }; + + // run scenario + const result = await testForLinterPromptResponse(promptReply); + // test results expect(result).to.equal(expectedResult, 'Expected promptToConfigureAvailableLinter to return true because the configuration was updated.'); - expect(linterInfo.testIsEnabled).to.equal(promptReply.enabled, 'LinterInfo test class was not updated as a result of the test.'); }); test('Linter is disabled after being prompted and "Disable " is selected', async () => { @@ -181,40 +169,11 @@ suite('Linter Availability Provider tests', () => { const promptReply = { title: 'Disable pylint', enabled: false }; const expectedResult = true; - // arrange - const appShellMock = TypeMoq.Mock.ofType(); - const installerMock = TypeMoq.Mock.ofType(); - const wkspcConfigSrvcMock = TypeMoq.Mock.ofType(); - - const configServiceMock = TypeMoq.Mock.ofType(); - - const linterInfo = new class extends LinterInfo { - public testIsEnabled: boolean = !promptReply.enabled; - - public async enableAsync(enabled: boolean, resource?: Uri): Promise { - this.testIsEnabled = enabled; - return Promise.resolve(); - } - - }(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']); - - appShellMock.setup(ap => ap.showInformationMessage( - TypeMoq.It.isValue(`Linter ${linterInfo.id} is available but not enabled.`), - TypeMoq.It.isAny(), - TypeMoq.It.isAny()) - ) - .returns(() => { - // tslint:disable-next-line:no-any - return promptReply as any; - }) - .verifiable(TypeMoq.Times.once()); - - // call - const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); - const result = await availabilityProvider.promptToConfigureAvailableLinter(linterInfo); + // run scenario + const result = await testForLinterPromptResponse(promptReply); + // test results expect(result).to.equal(expectedResult, 'Expected promptToConfigureAvailableLinter to return true because the configuration was updated.'); - expect(linterInfo.testIsEnabled).to.equal(promptReply.enabled, 'LinterInfo test class was not updated as a result of the test.'); }); test('Linter is left unconfigured after being prompted and the prompt is disabled without any selection made', async () => { @@ -222,258 +181,137 @@ suite('Linter Availability Provider tests', () => { const promptReply = undefined; const expectedResult = false; - // arrange - const appShellMock = TypeMoq.Mock.ofType(); - const installerMock = TypeMoq.Mock.ofType(); - const wkspcConfigSrvcMock = TypeMoq.Mock.ofType(); - - const configServiceMock = TypeMoq.Mock.ofType(); - - const linterInfo = new class extends LinterInfo { - public testIsEnabled: boolean = false; + // run scenario + const result = await testForLinterPromptResponse(promptReply); - public async enableAsync(enabled: boolean, resource?: Uri): Promise { - this.testIsEnabled = enabled; - return Promise.resolve(); - } + // test results + expect(result).to.equal(expectedResult, 'Expected promptToConfigureAvailableLinter to return true because the configuration was updated.'); + }); - }(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']); + // Options to test the implementation of the IAvailableLinterActivator. + // All options default to values that would otherwise allow the prompt to appear. + class AvailablityTestOverallOptions { + public jediEnabledValue: boolean = false; + public pylintUserEnabled?: boolean; + public pylintWorkspaceEnabled?: boolean; + public pylintWorkspaceFolderEnabled?: boolean; + public linterIsInstalled: boolean = true; + public promptReply?: { title: string; enabled: boolean }; + } + async function performTestOfOverallImplementation(options: AvailablityTestOverallOptions): Promise { + // arrange + const [appShellMock, installerMock, wkspcConfigSrvcMock, linterInfo] = getDependenciesForAvailabilityTests(); appShellMock.setup(ap => ap.showInformationMessage( TypeMoq.It.isValue(`Linter ${linterInfo.id} is available but not enabled.`), TypeMoq.It.isAny(), TypeMoq.It.isAny()) ) - .returns(() => { - // tslint:disable-next-line:no-any - return promptReply as any; - }) + // tslint:disable-next-line:no-any + .returns(() => options.promptReply as any) .verifiable(TypeMoq.Times.once()); - // call - const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); - const result = await availabilityProvider.promptToConfigureAvailableLinter(linterInfo); + installerMock.setup(im => im.isInstalled(linterInfo.product, TypeMoq.It.isAny())) + .returns(async () => options.linterIsInstalled) + .verifiable(TypeMoq.Times.once()); - expect(result).to.equal(expectedResult, 'Expected promptToConfigureAvailableLinter to return true because the configuration was updated.'); - }); + setupWorkspaceServiceForJediSettingTest(options.jediEnabledValue, wkspcConfigSrvcMock); + setupWorkspaceMockForLinterConfiguredTests( + options.pylintUserEnabled, + options.pylintWorkspaceEnabled, + options.pylintWorkspaceFolderEnabled, + wkspcConfigSrvcMock + ); + + // perform test + const availabilityProvider: IAvailableLinterActivator = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); + return availabilityProvider.promptIfLinterAvailable(linterInfo); + } test('Overall implementation does not change configuration when feature disabled', async () => { // set expectations - const jediEnabledValue = true; + const testOpts = new AvailablityTestOverallOptions(); + testOpts.jediEnabledValue = true; const expectedResult = false; // arrange - const appShellMock = TypeMoq.Mock.ofType(); - const installerMock = TypeMoq.Mock.ofType(); - const configServiceMock = TypeMoq.Mock.ofType(); - const wkspcConfigSrvcMock = setupWorkspaceServiceForJediSettingTest(jediEnabledValue); - const pylintInfo = new LinterInfo(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']); + const result = await performTestOfOverallImplementation(testOpts); // perform test - const availabilityProvider: IAvailableLinterActivator = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); - const result = await availabilityProvider.promptIfLinterAvailable(pylintInfo); - expect(expectedResult).to.equal(result, 'promptIfLinterAvailable should not change any configuration when python.jediEnabled is true.'); - wkspcConfigSrvcMock.verifyAll(); }); test('Overall implementation does not change configuration when linter is configured (enabled)', async () => { // set expectations - const jediEnabledValue = false; - const pylintUserEnabled = undefined; - const pylintWorkspaceEnabled = true; - const pylintWorkspaceFolderEnabled = undefined; + const testOpts = new AvailablityTestOverallOptions(); + testOpts.pylintWorkspaceEnabled = true; const expectedResult = false; // arrange - const appShellMock = TypeMoq.Mock.ofType(); - const installerMock = TypeMoq.Mock.ofType(); - const configServiceMock = TypeMoq.Mock.ofType(); - let wkspcConfigSrvcMock = setupWorkspaceServiceForJediSettingTest(jediEnabledValue); - wkspcConfigSrvcMock = setupWorkspaceMockForLinterConfiguredTests(pylintUserEnabled, pylintWorkspaceEnabled, pylintWorkspaceFolderEnabled, wkspcConfigSrvcMock); - const pylintInfo = new LinterInfo(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']); - - // perform test - const availabilityProvider: IAvailableLinterActivator = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); - const result = await availabilityProvider.promptIfLinterAvailable(pylintInfo); + const result = await performTestOfOverallImplementation(testOpts); // perform test expect(expectedResult).to.equal(result, 'Configuration should not change if the linter is configured in any way.'); - wkspcConfigSrvcMock.verifyAll(); }); test('Overall implementation does not change configuration when linter is configured (disabled)', async () => { // set expectations - const jediEnabledValue = false; - const pylintUserEnabled = undefined; - const pylintWorkspaceEnabled = true; - const pylintWorkspaceFolderEnabled = undefined; + const testOpts = new AvailablityTestOverallOptions(); + testOpts.pylintWorkspaceEnabled = false; const expectedResult = false; // arrange - const appShellMock = TypeMoq.Mock.ofType(); - const installerMock = TypeMoq.Mock.ofType(); - const configServiceMock = TypeMoq.Mock.ofType(); - let wkspcConfigSrvcMock = setupWorkspaceServiceForJediSettingTest(jediEnabledValue); - wkspcConfigSrvcMock = setupWorkspaceMockForLinterConfiguredTests(pylintUserEnabled, pylintWorkspaceEnabled, pylintWorkspaceFolderEnabled, wkspcConfigSrvcMock); - const pylintInfo = new LinterInfo(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']); - - // perform test - const availabilityProvider: IAvailableLinterActivator = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); - const result = await availabilityProvider.promptIfLinterAvailable(pylintInfo); + const result = await performTestOfOverallImplementation(testOpts); - // perform test expect(expectedResult).to.equal(result, 'Configuration should not change if the linter is disabled in any way.'); - wkspcConfigSrvcMock.verifyAll(); }); test('Overall implementation does not change configuration when linter is unavailable in current workspace environment', async () => { // set expectations - const jediEnabledValue = false; - const pylintUserEnabled = undefined; - const pylintWorkspaceEnabled = true; - const pylintWorkspaceFolderEnabled = undefined; - const linterIsInstalled = false; + const testOpts = new AvailablityTestOverallOptions(); + testOpts.pylintWorkspaceEnabled = true; const expectedResult = false; // arrange - const appShellMock = TypeMoq.Mock.ofType(); - const configServiceMock = TypeMoq.Mock.ofType(); - const pylintInfo = new LinterInfo(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']); - const installerMock = TypeMoq.Mock.ofType(); - installerMock.setup(im => im.isInstalled(pylintInfo.product, TypeMoq.It.isAny())) - .returns(async () => linterIsInstalled) - .verifiable(TypeMoq.Times.once()); - let wkspcConfigSrvcMock = setupWorkspaceServiceForJediSettingTest(jediEnabledValue); - wkspcConfigSrvcMock = setupWorkspaceMockForLinterConfiguredTests(pylintUserEnabled, pylintWorkspaceEnabled, pylintWorkspaceFolderEnabled, wkspcConfigSrvcMock); - - // perform test - const availabilityProvider: IAvailableLinterActivator = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); - const result = await availabilityProvider.promptIfLinterAvailable(pylintInfo); + const result = await performTestOfOverallImplementation(testOpts); expect(expectedResult).to.equal(result, 'Configuration should not change if the linter is unavailable in the current workspace environment.'); - wkspcConfigSrvcMock.verifyAll(); }); test('Overall implementation does not change configuration when user is prompted and prompt is dismissed', async () => { // set expectations - const jediEnabledValue = false; - const pylintUserEnabled = undefined; - const pylintWorkspaceEnabled = undefined; - const pylintWorkspaceFolderEnabled = undefined; - const linterIsInstalled = true; - const promptReply = undefined; + const testOpts = new AvailablityTestOverallOptions(); + testOpts.promptReply = undefined; // just being explicit for test readability - this is the default const expectedResult = false; // arrange - const configServiceMock = TypeMoq.Mock.ofType(); - const pylintInfo = new LinterInfo(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']); - const appShellMock = TypeMoq.Mock.ofType(); - appShellMock.setup(ap => ap.showInformationMessage( - TypeMoq.It.isValue(`Linter ${pylintInfo.id} is available but not enabled.`), - TypeMoq.It.isAny(), - TypeMoq.It.isAny()) - ) - // tslint:disable-next-line:no-any - .returns(() => promptReply as any) - .verifiable(TypeMoq.Times.once()); - const installerMock = TypeMoq.Mock.ofType(); - installerMock.setup(im => im.isInstalled(pylintInfo.product, TypeMoq.It.isAny())) - .returns(async () => linterIsInstalled) - .verifiable(TypeMoq.Times.once()); - let wkspcConfigSrvcMock = setupWorkspaceServiceForJediSettingTest(jediEnabledValue); - wkspcConfigSrvcMock = setupWorkspaceMockForLinterConfiguredTests(pylintUserEnabled, pylintWorkspaceEnabled, pylintWorkspaceFolderEnabled, wkspcConfigSrvcMock); - - // perform test - const availabilityProvider: IAvailableLinterActivator = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); - const result = await availabilityProvider.promptIfLinterAvailable(pylintInfo); + const result = await performTestOfOverallImplementation(testOpts); expect(expectedResult).to.equal(result, 'Configuration should not change if the user is prompted and they dismiss the prompt.'); - wkspcConfigSrvcMock.verifyAll(); - appShellMock.verifyAll(); - installerMock.verifyAll(); }); test('Overall implementation changes configuration when user is prompted and "Disable " is selected', async () => { // set expectations - const jediEnabledValue = false; - const pylintUserEnabled = undefined; - const pylintWorkspaceEnabled = undefined; - const pylintWorkspaceFolderEnabled = undefined; - const linterIsInstalled = true; - const promptReply = { title: 'Disable pylint', enabled: false }; + const testOpts = new AvailablityTestOverallOptions(); + testOpts.promptReply = { title: 'Disable pylint', enabled: false }; const expectedResult = true; // arrange - const configServiceMock = TypeMoq.Mock.ofType(); - const pylintInfo = new LinterInfo(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']); - const appShellMock = TypeMoq.Mock.ofType(); - appShellMock.setup(ap => ap.showInformationMessage( - TypeMoq.It.isValue(`Linter ${pylintInfo.id} is available but not enabled.`), - TypeMoq.It.isAny(), - TypeMoq.It.isAny()) - ) - .returns(() => { - // tslint:disable-next-line:no-any - return promptReply as any; - }) - .verifiable(TypeMoq.Times.once()); - const installerMock = TypeMoq.Mock.ofType(); - installerMock.setup(im => im.isInstalled(pylintInfo.product, TypeMoq.It.isAny())) - .returns(async () => linterIsInstalled) - .verifiable(TypeMoq.Times.once()); - let wkspcConfigSrvcMock = setupWorkspaceServiceForJediSettingTest(jediEnabledValue); - wkspcConfigSrvcMock = setupWorkspaceMockForLinterConfiguredTests(pylintUserEnabled, pylintWorkspaceEnabled, pylintWorkspaceFolderEnabled, wkspcConfigSrvcMock); - - // perform test - const availabilityProvider: IAvailableLinterActivator = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); - const result = await availabilityProvider.promptIfLinterAvailable(pylintInfo); + const result = await performTestOfOverallImplementation(testOpts); - expect(expectedResult).to.equal(result, 'Configuration should not change if the user is prompted and they dismiss the prompt.'); - wkspcConfigSrvcMock.verifyAll(); - appShellMock.verifyAll(); - installerMock.verifyAll(); + expect(expectedResult).to.equal(result, 'Configuration should change if the user is prompted and they choose to update the linter config.'); }); test('Overall implementation changes configuration when user is prompted and "Enable " is selected', async () => { // set expectations - const jediEnabledValue = false; - const pylintUserEnabled = undefined; - const pylintWorkspaceEnabled = undefined; - const pylintWorkspaceFolderEnabled = undefined; - const linterIsInstalled = true; - const promptReply = { title: 'Enable pylint', enabled: true }; + const testOpts = new AvailablityTestOverallOptions(); + testOpts.promptReply = { title: 'Enable pylint', enabled: true }; const expectedResult = true; // arrange - const configServiceMock = TypeMoq.Mock.ofType(); - const pylintInfo = new LinterInfo(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']); - const appShellMock = TypeMoq.Mock.ofType(); - appShellMock.setup(ap => ap.showInformationMessage( - TypeMoq.It.isValue(`Linter ${pylintInfo.id} is available but not enabled.`), - TypeMoq.It.isAny(), - TypeMoq.It.isAny()) - ) - .returns(() => { - // tslint:disable-next-line:no-any - return promptReply as any; - }) - .verifiable(TypeMoq.Times.once()); - const installerMock = TypeMoq.Mock.ofType(); - installerMock.setup(im => im.isInstalled(pylintInfo.product, TypeMoq.It.isAny())) - .returns(async () => linterIsInstalled) - .verifiable(TypeMoq.Times.once()); - let wkspcConfigSrvcMock = setupWorkspaceServiceForJediSettingTest(jediEnabledValue); - wkspcConfigSrvcMock = setupWorkspaceMockForLinterConfiguredTests(pylintUserEnabled, pylintWorkspaceEnabled, pylintWorkspaceFolderEnabled, wkspcConfigSrvcMock); + const result = await performTestOfOverallImplementation(testOpts); - // perform test - const availabilityProvider: IAvailableLinterActivator = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); - const result = await availabilityProvider.promptIfLinterAvailable(pylintInfo); - - expect(expectedResult).to.equal(result, 'Configuration should not change if the user is prompted and they dismiss the prompt.'); - wkspcConfigSrvcMock.verifyAll(); - appShellMock.verifyAll(); - installerMock.verifyAll(); + expect(expectedResult).to.equal(result, 'Configuration should change if the user is prompted and they choose to update the linter config.'); }); test('Discovery of linter is available in the environment returns true when it succeeds and is present', async () => { @@ -602,3 +440,18 @@ function setupWorkspaceServiceForJediSettingTest(jediEnabledValue: boolean, wksp .verifiable(TypeMoq.Times.once()); return wkspcConfigSrvcMock; } + +function getDependenciesForAvailabilityTests(): [ + TypeMoq.IMock, + TypeMoq.IMock, + TypeMoq.IMock, + LinterInfo +] { + const configServiceMock = TypeMoq.Mock.ofType(); + return [ + TypeMoq.Mock.ofType(), + TypeMoq.Mock.ofType(), + TypeMoq.Mock.ofType(), + new LinterInfo(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']) + ]; +} From dd0606b4917be6eda3ae08610805ec7fcb103437 Mon Sep 17 00:00:00 2001 From: Derek Keeler Date: Tue, 23 Oct 2018 14:01:51 -0700 Subject: [PATCH 12/16] PR suggestion: use IConfigurationService over IWorkspaceService - When determining the jediEnabled state. - Turns this feature on for anyone using the MPLS --- src/client/linters/linterAvailability.ts | 9 +- .../linters/linter.availability.unit.test.ts | 170 +++++++++--------- 2 files changed, 84 insertions(+), 95 deletions(-) diff --git a/src/client/linters/linterAvailability.ts b/src/client/linters/linterAvailability.ts index 692be9cccc2f..ec89c5b98453 100644 --- a/src/client/linters/linterAvailability.ts +++ b/src/client/linters/linterAvailability.ts @@ -7,7 +7,7 @@ import { inject, injectable } from 'inversify'; import { Uri } from 'vscode'; import { IApplicationShell, IWorkspaceService } from '../common/application/types'; import { traceError } from '../common/logger'; -import { IInstaller, Product } from '../common/types'; +import { IConfigurationService, IInstaller, Product } from '../common/types'; import { IAvailableLinterActivator, ILinterInfo } from './types'; @injectable() @@ -15,7 +15,8 @@ export class AvailableLinterActivator implements IAvailableLinterActivator { constructor( @inject(IApplicationShell) private appShell: IApplicationShell, @inject(IInstaller) private installer: IInstaller, - @inject(IWorkspaceService) private workspaceConfig: IWorkspaceService + @inject(IWorkspaceService) private workspaceConfig: IWorkspaceService, + @inject(IConfigurationService) private configService: IConfigurationService ) { } /** @@ -121,8 +122,6 @@ export class AvailableLinterActivator implements IAvailableLinterActivator { * @returns true if the global default for python.jediEnabled is false. */ public get isFeatureEnabled(): boolean { - const ws = this.workspaceConfig.getConfiguration('python'); - const jediEnabled = ws!.inspect('jediEnabled'); - return (!jediEnabled || jediEnabled.defaultValue === false); + return !this.configService.getSettings().jediEnabled; } } diff --git a/src/test/linters/linter.availability.unit.test.ts b/src/test/linters/linter.availability.unit.test.ts index 9d761cfe2cf5..d66691aa50b3 100644 --- a/src/test/linters/linter.availability.unit.test.ts +++ b/src/test/linters/linter.availability.unit.test.ts @@ -6,8 +6,12 @@ import { expect } from 'chai'; import * as TypeMoq from 'typemoq'; import { Uri, WorkspaceConfiguration } from 'vscode'; -import { IApplicationShell, IWorkspaceService } from '../../client/common/application/types'; -import { IConfigurationService, IInstaller, Product } from '../../client/common/types'; +import { + IApplicationShell, IWorkspaceService +} from '../../client/common/application/types'; +import { + IConfigurationService, IInstaller, IPythonSettings, Product +} from '../../client/common/types'; import { AvailableLinterActivator } from '../../client/linters/linterAvailability'; import { LinterInfo } from '../../client/linters/linterInfo'; import { IAvailableLinterActivator } from '../../client/linters/types'; @@ -21,15 +25,15 @@ suite('Linter Availability Provider tests', () => { const expectedResult = false; // arrange - const [appShellMock, installerMock, wkspcConfigSrvcMock] = getDependenciesForAvailabilityTests(); - setupWorkspaceServiceForJediSettingTest(jediEnabledValue, wkspcConfigSrvcMock); + const [appShellMock, installerMock, workspaceServiceMock, configServiceMock] = getDependenciesForAvailabilityTests(); + setupConfigurationServiceForJediSettingsTest(jediEnabledValue, configServiceMock); // call - const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); + const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, workspaceServiceMock.object, configServiceMock.object); // check expectaions - expect(availabilityProvider.isFeatureEnabled).is.equal(expectedResult, 'Avaialability feature should be disabled when python.jediEnabled defaults to true'); - wkspcConfigSrvcMock.verifyAll(); + expect(availabilityProvider.isFeatureEnabled).is.equal(expectedResult, 'Avaialability feature should be disabled when python.jediEnabled is true'); + workspaceServiceMock.verifyAll(); }); test('Availability feature is enabled when global default for jediEnabled=false.', async () => { @@ -38,13 +42,13 @@ suite('Linter Availability Provider tests', () => { const expectedResult = true; // arrange - const [appShellMock, installerMock, wkspcConfigSrvcMock] = getDependenciesForAvailabilityTests(); - setupWorkspaceServiceForJediSettingTest(jediEnabledValue, wkspcConfigSrvcMock); + const [appShellMock, installerMock, workspaceServiceMock, configServiceMock] = getDependenciesForAvailabilityTests(); + setupConfigurationServiceForJediSettingsTest(jediEnabledValue, configServiceMock); - const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); + const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, workspaceServiceMock.object, configServiceMock.object); expect(availabilityProvider.isFeatureEnabled).is.equal(expectedResult, 'Avaialability feature should be enabled when python.jediEnabled defaults to false'); - wkspcConfigSrvcMock.verifyAll(); + workspaceServiceMock.verifyAll(); }); test('Prompt will be performed when linter is not configured at all for the workspace, workspace-folder, or the user', async () => { @@ -54,15 +58,15 @@ suite('Linter Availability Provider tests', () => { const pylintWorkspaceFolderValue = undefined; const expectedResult = true; - const [appShellMock, installerMock, wkspcConfigSrvcMock, linterInfo] = getDependenciesForAvailabilityTests(); - setupWorkspaceMockForLinterConfiguredTests(pylintUserValue, pylintWorkspaceValue, pylintWorkspaceFolderValue, wkspcConfigSrvcMock); + const [appShellMock, installerMock, workspaceServiceMock, configServiceMock, linterInfo] = getDependenciesForAvailabilityTests(); + setupWorkspaceMockForLinterConfiguredTests(pylintUserValue, pylintWorkspaceValue, pylintWorkspaceFolderValue, workspaceServiceMock); - const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); + const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, workspaceServiceMock.object, configServiceMock.object); const result = availabilityProvider.isLinterUsingDefaultConfiguration(linterInfo); expect(result).to.equal(expectedResult, 'Linter is unconfigured but prompt did not get raised'); - wkspcConfigSrvcMock.verifyAll(); + workspaceServiceMock.verifyAll(); }); test('No prompt performed when linter is configured as enabled for the workspace', async () => { @@ -72,14 +76,14 @@ suite('Linter Availability Provider tests', () => { const pylintWorkspaceFolderValue = undefined; const expectedResult = false; - const [appShellMock, installerMock, wkspcConfigSrvcMock, linterInfo] = getDependenciesForAvailabilityTests(); - setupWorkspaceMockForLinterConfiguredTests(pylintUserValue, pylintWorkspaceValue, pylintWorkspaceFolderValue, wkspcConfigSrvcMock); + const [appShellMock, installerMock, workspaceServiceMock, configServiceMock, linterInfo] = getDependenciesForAvailabilityTests(); + setupWorkspaceMockForLinterConfiguredTests(pylintUserValue, pylintWorkspaceValue, pylintWorkspaceFolderValue, workspaceServiceMock); - const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); + const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, workspaceServiceMock.object, configServiceMock.object); const result = availabilityProvider.isLinterUsingDefaultConfiguration(linterInfo); expect(result).to.equal(expectedResult, 'Available linter prompt should not be shown when linter is configured for workspace.'); - wkspcConfigSrvcMock.verifyAll(); + workspaceServiceMock.verifyAll(); }); test('No prompt performed when linter is configured as enabled for the entire user', async () => { @@ -90,13 +94,13 @@ suite('Linter Availability Provider tests', () => { const expectedResult = false; // arrange - const [appShellMock, installerMock, wkspcConfigSrvcMock, linterInfo] = getDependenciesForAvailabilityTests(); - setupWorkspaceMockForLinterConfiguredTests(pylintUserValue, pylintWorkspaceValue, pylintWorkspaceFolderValue, wkspcConfigSrvcMock); - const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); + const [appShellMock, installerMock, workspaceServiceMock, configServiceMock, linterInfo] = getDependenciesForAvailabilityTests(); + setupWorkspaceMockForLinterConfiguredTests(pylintUserValue, pylintWorkspaceValue, pylintWorkspaceFolderValue, workspaceServiceMock); + const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, workspaceServiceMock.object, configServiceMock.object); const result = availabilityProvider.isLinterUsingDefaultConfiguration(linterInfo); expect(result).to.equal(expectedResult, 'Available linter prompt should not be shown when linter is configured for user.'); - wkspcConfigSrvcMock.verifyAll(); + workspaceServiceMock.verifyAll(); }); test('No prompt performed when linter is configured as enabled for the workspace-folder', async () => { @@ -107,18 +111,18 @@ suite('Linter Availability Provider tests', () => { const expectedResult = false; // arrange - const [appShellMock, installerMock, wkspcConfigSrvcMock, linterInfo] = getDependenciesForAvailabilityTests(); - setupWorkspaceMockForLinterConfiguredTests(pylintUserValue, pylintWorkspaceValue, pylintWorkspaceFolderValue, wkspcConfigSrvcMock); - const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); + const [appShellMock, installerMock, workspaceServiceMock, configServiceMock, linterInfo] = getDependenciesForAvailabilityTests(); + setupWorkspaceMockForLinterConfiguredTests(pylintUserValue, pylintWorkspaceValue, pylintWorkspaceFolderValue, workspaceServiceMock); + const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, workspaceServiceMock.object, configServiceMock.object); const result = availabilityProvider.isLinterUsingDefaultConfiguration(linterInfo); expect(result).to.equal(expectedResult, 'Available linter prompt should not be shown when linter is configured for workspace-folder.'); - wkspcConfigSrvcMock.verifyAll(); + workspaceServiceMock.verifyAll(); }); async function testForLinterPromptResponse(promptReply: { title: string; enabled: boolean } | undefined): Promise { // arrange - const [appShellMock, installerMock, wkspcConfigSrvcMock] = getDependenciesForAvailabilityTests(); + const [appShellMock, installerMock, workspaceServiceMock] = getDependenciesForAvailabilityTests(); const configServiceMock = TypeMoq.Mock.ofType(); const linterInfo = new class extends LinterInfo { @@ -143,7 +147,7 @@ suite('Linter Availability Provider tests', () => { .verifiable(TypeMoq.Times.once()); // perform test - const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); + const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, workspaceServiceMock.object, configServiceMock.object); const result = await availabilityProvider.promptToConfigureAvailableLinter(linterInfo); if (promptReply) { expect(linterInfo.testIsEnabled).to.equal(promptReply.enabled, 'LinterInfo test class was not updated as a result of the test.'); @@ -201,7 +205,7 @@ suite('Linter Availability Provider tests', () => { async function performTestOfOverallImplementation(options: AvailablityTestOverallOptions): Promise { // arrange - const [appShellMock, installerMock, wkspcConfigSrvcMock, linterInfo] = getDependenciesForAvailabilityTests(); + const [appShellMock, installerMock, workspaceServiceMock, configServiceMock, linterInfo] = getDependenciesForAvailabilityTests(); appShellMock.setup(ap => ap.showInformationMessage( TypeMoq.It.isValue(`Linter ${linterInfo.id} is available but not enabled.`), TypeMoq.It.isAny(), @@ -215,16 +219,16 @@ suite('Linter Availability Provider tests', () => { .returns(async () => options.linterIsInstalled) .verifiable(TypeMoq.Times.once()); - setupWorkspaceServiceForJediSettingTest(options.jediEnabledValue, wkspcConfigSrvcMock); + setupConfigurationServiceForJediSettingsTest(options.jediEnabledValue, configServiceMock); setupWorkspaceMockForLinterConfiguredTests( options.pylintUserEnabled, options.pylintWorkspaceEnabled, options.pylintWorkspaceFolderEnabled, - wkspcConfigSrvcMock + workspaceServiceMock ); // perform test - const availabilityProvider: IAvailableLinterActivator = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); + const availabilityProvider: IAvailableLinterActivator = new AvailableLinterActivator(appShellMock.object, installerMock.object, workspaceServiceMock.object, configServiceMock.object); return availabilityProvider.promptIfLinterAvailable(linterInfo); } @@ -320,20 +324,12 @@ suite('Linter Availability Provider tests', () => { const expectedResult = true; // arrange - const appShellMock = TypeMoq.Mock.ofType(); - const installerMock = TypeMoq.Mock.ofType(); - const configServiceMock = TypeMoq.Mock.ofType(); - const wkspcConfigSrvcMock = TypeMoq.Mock.ofType(); - - const pylintInfo = new LinterInfo(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']); - - installerMock.setup(im => im.isInstalled(pylintInfo.product, TypeMoq.It.isAny())) - .returns(async () => linterIsInstalled) - .verifiable(TypeMoq.Times.once()); + const [appShellMock, installerMock, workspaceServiceMock, configServiceMock, linterInfo] = getDependenciesForAvailabilityTests(); + setupInstallerForAvailabilityTest(linterInfo, linterIsInstalled, installerMock); // perform test - const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); - const result = await availabilityProvider.isLinterAvailable(pylintInfo.product); + const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, workspaceServiceMock.object, configServiceMock.object); + const result = await availabilityProvider.isLinterAvailable(linterInfo.product); expect(result).to.equal(expectedResult, 'Expected promptToConfigureAvailableLinter to return true because the configuration was updated.'); installerMock.verifyAll(); @@ -345,20 +341,12 @@ suite('Linter Availability Provider tests', () => { const expectedResult = false; // arrange - const appShellMock = TypeMoq.Mock.ofType(); - const installerMock = TypeMoq.Mock.ofType(); - const configServiceMock = TypeMoq.Mock.ofType(); - const wkspcConfigSrvcMock = TypeMoq.Mock.ofType(); - - const pylintInfo = new LinterInfo(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']); - - installerMock.setup(im => im.isInstalled(pylintInfo.product, TypeMoq.It.isAny())) - .returns(async () => linterIsInstalled) - .verifiable(TypeMoq.Times.once()); + const [appShellMock, installerMock, workspaceServiceMock, configServiceMock, linterInfo] = getDependenciesForAvailabilityTests(); + setupInstallerForAvailabilityTest(linterInfo, linterIsInstalled, installerMock); // perform test - const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); - const result = await availabilityProvider.isLinterAvailable(pylintInfo.product); + const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, workspaceServiceMock.object, configServiceMock.object); + const result = await availabilityProvider.isLinterAvailable(linterInfo.product); expect(result).to.equal(expectedResult, 'Expected promptToConfigureAvailableLinter to return true because the configuration was updated.'); installerMock.verifyAll(); @@ -369,20 +357,14 @@ suite('Linter Availability Provider tests', () => { const expectedResult = false; // arrange - const appShellMock = TypeMoq.Mock.ofType(); - const installerMock = TypeMoq.Mock.ofType(); - const configServiceMock = TypeMoq.Mock.ofType(); - const wkspcConfigSrvcMock = TypeMoq.Mock.ofType(); - - const pylintInfo = new LinterInfo(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']); - - installerMock.setup(im => im.isInstalled(pylintInfo.product, TypeMoq.It.isAny())) + const [appShellMock, installerMock, workspaceServiceMock, configServiceMock, linterInfo] = getDependenciesForAvailabilityTests(); + installerMock.setup(im => im.isInstalled(linterInfo.product, TypeMoq.It.isAny())) .returns(() => Promise.reject('error testfail')) .verifiable(TypeMoq.Times.once()); // perform test - const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, wkspcConfigSrvcMock.object); - const result = await availabilityProvider.isLinterAvailable(pylintInfo.product); + const availabilityProvider = new AvailableLinterActivator(appShellMock.object, installerMock.object, workspaceServiceMock.object, configServiceMock.object); + const result = await availabilityProvider.isLinterAvailable(linterInfo.product); expect(result).to.equal(expectedResult, 'Expected promptToConfigureAvailableLinter to return true because the configuration was updated.'); installerMock.verifyAll(); @@ -393,10 +375,10 @@ function setupWorkspaceMockForLinterConfiguredTests( enabledForUser: boolean | undefined, enabeldForWorkspace: boolean | undefined, enabledForWorkspaceFolder: boolean | undefined, - wkspcConfigSrvcMock?: TypeMoq.IMock): TypeMoq.IMock { + workspaceServiceMock?: TypeMoq.IMock): TypeMoq.IMock { - if (!wkspcConfigSrvcMock) { - wkspcConfigSrvcMock = TypeMoq.Mock.ofType(); + if (!workspaceServiceMock) { + workspaceServiceMock = TypeMoq.Mock.ofType(); } const workspaceConfiguration = TypeMoq.Mock.ofType(); workspaceConfiguration.setup(wc => wc.inspect(TypeMoq.It.isValue('pylintEnabled'))) @@ -411,40 +393,47 @@ function setupWorkspaceMockForLinterConfiguredTests( }) .verifiable(TypeMoq.Times.once()); - wkspcConfigSrvcMock.setup(ws => ws.getConfiguration(TypeMoq.It.isValue('python.linting'), TypeMoq.It.isAny())) + workspaceServiceMock.setup(ws => ws.getConfiguration(TypeMoq.It.isValue('python.linting'), TypeMoq.It.isAny())) .returns(() => workspaceConfiguration.object) .verifiable(TypeMoq.Times.once()); - return wkspcConfigSrvcMock; + return workspaceServiceMock; } -function setupWorkspaceServiceForJediSettingTest(jediEnabledValue: boolean, wkspcConfigSrvcMock?: TypeMoq.IMock): TypeMoq.IMock { +function setupConfigurationServiceForJediSettingsTest( + jediEnabledValue: boolean, + configServiceMock: TypeMoq.IMock +): [ + TypeMoq.IMock, + TypeMoq.IMock + ] { - if (!wkspcConfigSrvcMock) { - wkspcConfigSrvcMock = TypeMoq.Mock.ofType(); + if (!configServiceMock) { + configServiceMock = TypeMoq.Mock.ofType(); } - const workspaceConfiguration = TypeMoq.Mock.ofType(); - workspaceConfiguration.setup(wc => wc.inspect(TypeMoq.It.isValue('jediEnabled'))) - .returns(() => { - return { - key: '', - globalValue: undefined, - defaultValue: jediEnabledValue, - workspaceFolderValue: undefined, - workspaceValue: undefined - }; - }) - .verifiable(TypeMoq.Times.once()); - wkspcConfigSrvcMock.setup(ws => ws.getConfiguration(TypeMoq.It.isValue('python'))) - .returns(() => workspaceConfiguration.object) + const pythonSettings = TypeMoq.Mock.ofType(); + pythonSettings.setup(ps => ps.jediEnabled).returns(() => jediEnabledValue); + + configServiceMock.setup(cs => cs.getSettings()).returns(() => pythonSettings.object); + return [configServiceMock, pythonSettings]; +} + +function setupInstallerForAvailabilityTest(linterInfo: LinterInfo, linterIsInstalled: boolean, installerMock): TypeMoq.IMock { + if (!installerMock) { + installerMock = TypeMoq.Mock.ofType(); + } + installerMock.setup(im => im.isInstalled(linterInfo.product, TypeMoq.It.isAny())) + .returns(async () => linterIsInstalled) .verifiable(TypeMoq.Times.once()); - return wkspcConfigSrvcMock; + + return installerMock; } function getDependenciesForAvailabilityTests(): [ TypeMoq.IMock, TypeMoq.IMock, TypeMoq.IMock, + TypeMoq.IMock, LinterInfo ] { const configServiceMock = TypeMoq.Mock.ofType(); @@ -452,6 +441,7 @@ function getDependenciesForAvailabilityTests(): [ TypeMoq.Mock.ofType(), TypeMoq.Mock.ofType(), TypeMoq.Mock.ofType(), + TypeMoq.Mock.ofType(), new LinterInfo(Product.pylint, 'pylint', configServiceMock.object, ['.pylintrc', 'pylintrc']) ]; } From 2ad4263b3e9e4a155844bf9db503803a63bd0581 Mon Sep 17 00:00:00 2001 From: Derek Keeler Date: Tue, 23 Oct 2018 18:34:12 -0700 Subject: [PATCH 13/16] Fix code to match expectations of other tests - ensure only valid linters can be set (we can't default to pylint anymore) - ensure await is called for isLintingEnabled. --- package.json | 2 +- src/client/linters/linterManager.ts | 36 +++++++++++++++++++-------- src/test/linters/lint.manager.test.ts | 4 +-- 3 files changed, 29 insertions(+), 13 deletions(-) diff --git a/package.json b/package.json index 6501aae56d20..abb50ce13654 100644 --- a/package.json +++ b/package.json @@ -1257,7 +1257,7 @@ }, "python.linting.pylintEnabled": { "type": "boolean", - "default": false, + "default": true, "description": "Whether to lint Python files using pylint.", "scope": "resource" }, diff --git a/src/client/linters/linterManager.ts b/src/client/linters/linterManager.ts index 67344c12407b..08c2a3740247 100644 --- a/src/client/linters/linterManager.ts +++ b/src/client/linters/linterManager.ts @@ -81,20 +81,36 @@ export class LinterManager implements ILinterManager { if (!silent) { await this.enableUnconfiguredLinters(resource); } - return this.linters.filter(x => x.isEnabled(resource)); + return this.linters.filter(x => { + let enabled = x.isEnabled(resource); + if (x.id === 'pylint') { + enabled = enabled === true; + } + return enabled === true; + }); } public async setActiveLintersAsync(products: Product[], resource?: Uri): Promise { - const active = await this.getActiveLinters(true, resource); - for (const x of active) { - await x.enableAsync(false, resource); - } - if (products.length > 0) { - const toActivate = this.linters.filter(x => products.findIndex(p => x.product === p) >= 0); - for (const x of toActivate) { - await x.enableAsync(true, resource); + // ensure we only allow valid linters to be set, otherwise leave things alone. + // filter out any invalid products: + const validProducts = products.filter(product => { + const foundIndex = this.linters.findIndex(validLinter => validLinter.product === product); + return foundIndex !== -1; + }); + + // if we have valid linter product(s), enable only those + if (validProducts.length > 0) { + const active = await this.getActiveLinters(true, resource); + for (const x of active) { + await x.enableAsync(false, resource); + } + if (products.length > 0) { + const toActivate = this.linters.filter(x => products.findIndex(p => x.product === p) >= 0); + for (const x of toActivate) { + await x.enableAsync(true, resource); + } + await this.enableLintingAsync(true, resource); } - await this.enableLintingAsync(true, resource); } } diff --git a/src/test/linters/lint.manager.test.ts b/src/test/linters/lint.manager.test.ts index 3d0abb77d7be..e6a0a3a63361 100644 --- a/src/test/linters/lint.manager.test.ts +++ b/src/test/linters/lint.manager.test.ts @@ -81,9 +81,9 @@ suite('Linting - Manager', () => { test('Enable/disable linting', async () => { await lm.enableLintingAsync(false); - assert.equal(lm.isLintingEnabled(true), false, 'Linting not disabled'); + assert.equal(await lm.isLintingEnabled(true), false, 'Linting not disabled'); await lm.enableLintingAsync(true); - assert.equal(lm.isLintingEnabled(true), true, 'Linting not enabled'); + assert.equal(await lm.isLintingEnabled(true), true, 'Linting not enabled'); }); test('Set single linter', async () => { From 7b831788ba0dd7d8c8a04079b303aff6fe1128e9 Mon Sep 17 00:00:00 2001 From: Derek Keeler Date: Wed, 24 Oct 2018 10:58:09 -0700 Subject: [PATCH 14/16] Correct tests in linter provider to handle async callbacks - update test suite setup - include the new linter availability dependancy - correct linterProvider to be async onDocumentSave - remove previous debugging-only code from linterManager! --- src/client/linters/linterManager.ts | 8 +------- src/client/providers/linterProvider.ts | 2 +- src/test/linters/lint.provider.test.ts | 27 ++++++++++++++++++++++---- 3 files changed, 25 insertions(+), 12 deletions(-) diff --git a/src/client/linters/linterManager.ts b/src/client/linters/linterManager.ts index 08c2a3740247..1778736f5e79 100644 --- a/src/client/linters/linterManager.ts +++ b/src/client/linters/linterManager.ts @@ -81,13 +81,7 @@ export class LinterManager implements ILinterManager { if (!silent) { await this.enableUnconfiguredLinters(resource); } - return this.linters.filter(x => { - let enabled = x.isEnabled(resource); - if (x.id === 'pylint') { - enabled = enabled === true; - } - return enabled === true; - }); + return this.linters.filter(x => x.isEnabled(resource)); } public async setActiveLintersAsync(products: Product[], resource?: Uri): Promise { diff --git a/src/client/providers/linterProvider.ts b/src/client/providers/linterProvider.ts index 952d955aa69d..b9e0df0eb0c6 100644 --- a/src/client/providers/linterProvider.ts +++ b/src/client/providers/linterProvider.ts @@ -85,7 +85,7 @@ export class LinterProvider implements Disposable { private async onDocumentSaved(document: TextDocument): Promise { const settings = this.configuration.getSettings(document.uri); if (document.languageId === 'python' && settings.linting.enabled && settings.linting.lintOnSave) { - this.engine.lintDocument(document, 'save').ignoreErrors(); + await this.engine.lintDocument(document, 'save'); return; } diff --git a/src/test/linters/lint.provider.test.ts b/src/test/linters/lint.provider.test.ts index 07de8b37ad45..9466788eae56 100644 --- a/src/test/linters/lint.provider.test.ts +++ b/src/test/linters/lint.provider.test.ts @@ -4,15 +4,23 @@ import { Container } from 'inversify'; import * as TypeMoq from 'typemoq'; import * as vscode from 'vscode'; -import { IDocumentManager } from '../../client/common/application/types'; +import { + IApplicationShell, IDocumentManager, IWorkspaceService +} from '../../client/common/application/types'; import { IFileSystem } from '../../client/common/platform/types'; -import { IConfigurationService, ILintingSettings, IPythonSettings, Product } from '../../client/common/types'; +import { + IConfigurationService, IInstaller, ILintingSettings, + IPythonSettings, Product +} from '../../client/common/types'; import { createDeferred } from '../../client/common/utils/async'; import { IInterpreterService } from '../../client/interpreter/contracts'; import { ServiceContainer } from '../../client/ioc/container'; import { ServiceManager } from '../../client/ioc/serviceManager'; +import { AvailableLinterActivator } from '../../client/linters/linterAvailability'; import { LinterManager } from '../../client/linters/linterManager'; -import { ILinterManager, ILintingEngine } from '../../client/linters/types'; +import { + IAvailableLinterActivator, ILinterManager, ILintingEngine +} from '../../client/linters/types'; import { LinterProvider } from '../../client/providers/linterProvider'; import { initialize } from '../initialize'; @@ -29,6 +37,9 @@ suite('Linting - Provider', () => { let emitter: vscode.EventEmitter; let document: TypeMoq.IMock; let fs: TypeMoq.IMock; + let appShell: TypeMoq.IMock; + let linterInstaller: TypeMoq.IMock; + let workspaceService: TypeMoq.IMock; suiteSetup(initialize); setup(async () => { @@ -63,6 +74,14 @@ suite('Linting - Provider', () => { configService.setup(x => x.getSettings(TypeMoq.It.isAny())).returns(() => settings.object); serviceManager.addSingletonInstance(IConfigurationService, configService.object); + appShell = TypeMoq.Mock.ofType(); + linterInstaller = TypeMoq.Mock.ofType(); + workspaceService = TypeMoq.Mock.ofType(); + serviceManager.addSingletonInstance(IApplicationShell, appShell.object); + serviceManager.addSingletonInstance(IInstaller, linterInstaller.object); + serviceManager.addSingletonInstance(IWorkspaceService, workspaceService.object); + serviceManager.add(IAvailableLinterActivator, AvailableLinterActivator); + lm = new LinterManager(serviceContainer); serviceManager.addSingletonInstance(ILinterManager, lm); emitter = new vscode.EventEmitter(); @@ -80,7 +99,7 @@ suite('Linting - Provider', () => { engine.verify(x => x.lintDocument(document.object, 'auto'), TypeMoq.Times.once()); }); - test('Lint on save file', () => { + test('Lint on save file', async () => { docManager.setup(x => x.onDidSaveTextDocument).returns(() => emitter.event); document.setup(x => x.uri).returns(() => vscode.Uri.file('test.py')); document.setup(x => x.languageId).returns(() => 'python'); From 9ba3a125f09de7e3841705e871739bed316496ae Mon Sep 17 00:00:00 2001 From: Derek Keeler Date: Wed, 24 Oct 2018 11:56:29 -0700 Subject: [PATCH 15/16] Problem with `linterProvider::onDocumentSave` caught and corrected in PR - Linting cannot block saving --- src/client/providers/linterProvider.ts | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/client/providers/linterProvider.ts b/src/client/providers/linterProvider.ts index b9e0df0eb0c6..19f7d5a9c95b 100644 --- a/src/client/providers/linterProvider.ts +++ b/src/client/providers/linterProvider.ts @@ -8,6 +8,7 @@ import { ConfigurationTarget, Disposable, ExtensionContext, TextDocument, Uri, workspace } from 'vscode'; +import '../../../typings/extensions'; import { IDocumentManager } from '../common/application/types'; import { ConfigSettingMonitor } from '../common/configSettingMonitor'; import { isTestExecution } from '../common/constants'; @@ -43,7 +44,7 @@ export class LinterProvider implements Disposable { this.documents.onDidOpenTextDocument(e => this.onDocumentOpened(e), this.context.subscriptions); this.documents.onDidCloseTextDocument(e => this.onDocumentClosed(e), this.context.subscriptions); - this.documents.onDidSaveTextDocument(async (e) => this.onDocumentSaved(e), this.context.subscriptions); + this.documents.onDidSaveTextDocument(e => this.onDocumentSaved(e), this.context.subscriptions); this.configMonitor = new ConfigSettingMonitor('linting'); this.configMonitor.on('change', this.lintSettingsChangedHandler.bind(this)); @@ -82,19 +83,21 @@ export class LinterProvider implements Disposable { this.engine.lintDocument(document, 'auto').ignoreErrors(); } - private async onDocumentSaved(document: TextDocument): Promise { + private onDocumentSaved(document: TextDocument): void { const settings = this.configuration.getSettings(document.uri); if (document.languageId === 'python' && settings.linting.enabled && settings.linting.lintOnSave) { - await this.engine.lintDocument(document, 'save'); + this.engine.lintDocument(document, 'save').ignoreErrors(); return; } - const linters = await this.linterManager.getActiveLinters(false, document.uri); - const fileName = path.basename(document.uri.fsPath).toLowerCase(); - const watchers = linters.filter((info) => info.configFileNames.indexOf(fileName) >= 0); - if (watchers.length > 0) { - setTimeout(() => this.engine.lintOpenPythonFiles(), 1000); - } + this.linterManager.getActiveLinters(false, document.uri) + .then((linters) => { + const fileName = path.basename(document.uri.fsPath).toLowerCase(); + const watchers = linters.filter((info) => info.configFileNames.indexOf(fileName) >= 0); + if (watchers.length > 0) { + setTimeout(() => this.engine.lintOpenPythonFiles(), 1000); + } + }).ignoreErrors(); } private onDocumentClosed(document: TextDocument) { From 350e14306112da448ebcd07c0f3cbe6d917f710e Mon Sep 17 00:00:00 2001 From: Derek Keeler Date: Wed, 24 Oct 2018 14:08:04 -0700 Subject: [PATCH 16/16] Use correct path to extensions dependancy --- src/client/providers/linterProvider.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/providers/linterProvider.ts b/src/client/providers/linterProvider.ts index 19f7d5a9c95b..70a7c49b3916 100644 --- a/src/client/providers/linterProvider.ts +++ b/src/client/providers/linterProvider.ts @@ -8,10 +8,10 @@ import { ConfigurationTarget, Disposable, ExtensionContext, TextDocument, Uri, workspace } from 'vscode'; -import '../../../typings/extensions'; import { IDocumentManager } from '../common/application/types'; import { ConfigSettingMonitor } from '../common/configSettingMonitor'; import { isTestExecution } from '../common/constants'; +import '../common/extensions'; import { IFileSystem } from '../common/platform/types'; import { IConfigurationService } from '../common/types'; import { IInterpreterService } from '../interpreter/contracts';