From 6545a3daf0af4b8174ed528955509982fff767cb Mon Sep 17 00:00:00 2001 From: Don Jayamanne Date: Tue, 6 Nov 2018 14:20:29 -0800 Subject: [PATCH 1/2] Fixes to pylint checks --- src/client/common/application/types.ts | 7 ++++ src/client/common/application/workspace.ts | 44 +++++++++++----------- src/client/linters/linterInfo.ts | 21 ++++++++++- src/client/linters/linterManager.ts | 43 +++++++++++---------- src/test/linters/lint.args.test.ts | 2 +- src/test/linters/lint.commands.test.ts | 5 ++- src/test/linters/lint.manager.test.ts | 5 ++- src/test/linters/lint.manager.unit.test.ts | 14 +++---- src/test/linters/lint.provider.test.ts | 2 +- src/test/linters/lint.test.ts | 3 +- src/test/linters/pylint.test.ts | 2 +- 11 files changed, 89 insertions(+), 59 deletions(-) diff --git a/src/client/common/application/types.ts b/src/client/common/application/types.ts index 2e1e6aa6d173..c2f62db37405 100644 --- a/src/client/common/application/types.ts +++ b/src/client/common/application/types.ts @@ -556,6 +556,13 @@ export interface IWorkspaceService { */ getWorkspaceFolder(uri: Uri): WorkspaceFolder | undefined; + /** + * Generate a key that's unique to the workspace folder (could be fsPath). + * @param {(Uri | undefined)} resource + * @returns {string} + * @memberof IWorkspaceService + */ + getWorkspaceFolderIdentifier(resource: Uri | undefined): string; /** * Returns a path that is relative to the workspace folder or folders. * diff --git a/src/client/common/application/workspace.ts b/src/client/common/application/workspace.ts index 08bd950fe655..96285a73320d 100644 --- a/src/client/common/application/workspace.ts +++ b/src/client/common/application/workspace.ts @@ -2,43 +2,43 @@ // Licensed under the MIT License. import { injectable } from 'inversify'; -import * as vscode from 'vscode'; -import { ConfigurationChangeEvent } from 'vscode'; +import { CancellationToken, ConfigurationChangeEvent, Event, FileSystemWatcher, GlobPattern, Uri, workspace, WorkspaceConfiguration, WorkspaceFolder, WorkspaceFoldersChangeEvent } from 'vscode'; import { IWorkspaceService } from './types'; @injectable() export class WorkspaceService implements IWorkspaceService { - public get onDidChangeConfiguration(): vscode.Event { - return vscode.workspace.onDidChangeConfiguration; + public get onDidChangeConfiguration(): Event { + return workspace.onDidChangeConfiguration; } public get rootPath(): string | undefined { - return Array.isArray(vscode.workspace.workspaceFolders) ? vscode.workspace.workspaceFolders[0].uri.fsPath : undefined; + return Array.isArray(workspace.workspaceFolders) ? workspace.workspaceFolders[0].uri.fsPath : undefined; } - public get workspaceFolders(): vscode.WorkspaceFolder[] | undefined { - return vscode.workspace.workspaceFolders; + public get workspaceFolders(): WorkspaceFolder[] | undefined { + return workspace.workspaceFolders; } - public get onDidChangeWorkspaceFolders(): vscode.Event { - return vscode.workspace.onDidChangeWorkspaceFolders; + public get onDidChangeWorkspaceFolders(): Event { + return workspace.onDidChangeWorkspaceFolders; } public get hasWorkspaceFolders() { - return Array.isArray(vscode.workspace.workspaceFolders) && vscode.workspace.workspaceFolders.length > 0; + return Array.isArray(workspace.workspaceFolders) && workspace.workspaceFolders.length > 0; } - public getConfiguration(section?: string, resource?: vscode.Uri): vscode.WorkspaceConfiguration { - return vscode.workspace.getConfiguration(section, resource); + public getConfiguration(section?: string, resource?: Uri): WorkspaceConfiguration { + return workspace.getConfiguration(section, resource); } - public getWorkspaceFolder(uri: vscode.Uri): vscode.WorkspaceFolder | undefined { - return vscode.workspace.getWorkspaceFolder(uri); + public getWorkspaceFolder(uri: Uri): WorkspaceFolder | undefined { + return workspace.getWorkspaceFolder(uri); } - public asRelativePath(pathOrUri: string | vscode.Uri, includeWorkspaceFolder?: boolean): string { - return vscode.workspace.asRelativePath(pathOrUri, includeWorkspaceFolder); + public asRelativePath(pathOrUri: string | Uri, includeWorkspaceFolder?: boolean): string { + return workspace.asRelativePath(pathOrUri, includeWorkspaceFolder); } - public createFileSystemWatcher(globPattern: vscode.GlobPattern, ignoreCreateEvents?: boolean, ignoreChangeEvents?: boolean, ignoreDeleteEvents?: boolean): vscode.FileSystemWatcher { - return vscode.workspace.createFileSystemWatcher(globPattern, ignoreChangeEvents, ignoreChangeEvents, ignoreDeleteEvents); + public createFileSystemWatcher(globPattern: GlobPattern, ignoreCreateEvents?: boolean, ignoreChangeEvents?: boolean, ignoreDeleteEvents?: boolean): FileSystemWatcher { + return workspace.createFileSystemWatcher(globPattern, ignoreChangeEvents, ignoreChangeEvents, ignoreDeleteEvents); } - public findFiles(include: vscode.GlobPattern, exclude?: vscode.GlobPattern, maxResults?: number, token?: vscode.CancellationToken): Thenable { - return vscode.workspace.findFiles(include, exclude, maxResults, token); + public findFiles(include: GlobPattern, exclude?: GlobPattern, maxResults?: number, token?: CancellationToken): Thenable { + return workspace.findFiles(include, exclude, maxResults, token); } - public get onDidSaveTextDocument(): vscode.Event { - return vscode.workspace.onDidSaveTextDocument; + public getWorkspaceFolderIdentifier(resource: Uri): string { + const workspaceFolder = resource ? workspace.getWorkspaceFolder(resource) : undefined; + return workspaceFolder ? workspaceFolder.uri.fsPath : ''; } } diff --git a/src/client/linters/linterInfo.ts b/src/client/linters/linterInfo.ts index 055d76311d39..09cb3b82ba23 100644 --- a/src/client/linters/linterInfo.ts +++ b/src/client/linters/linterInfo.ts @@ -3,6 +3,7 @@ import * as path from 'path'; import { Uri } from 'vscode'; +import { IWorkspaceService } from '../common/application/types'; import { ExecutionInfo, IConfigurationService, Product } from '../common/types'; import { ILinterInfo, LinterId } from './types'; @@ -11,7 +12,7 @@ export class LinterInfo implements ILinterInfo { private _product: Product; private _configFileNames: string[]; - constructor(product: Product, id: LinterId, private configService: IConfigurationService, configFileNames: string[] = []) { + constructor(product: Product, id: LinterId, protected configService: IConfigurationService, configFileNames: string[] = []) { this._product = product; this._id = id; this._configFileNames = configFileNames; @@ -67,3 +68,21 @@ export class LinterInfo implements ILinterInfo { return { execPath, moduleName, args, product: this.product }; } } + +export class PylintLinterInfo extends LinterInfo { + constructor(configService: IConfigurationService, private readonly workspaceService: IWorkspaceService, configFileNames: string[] = []) { + super(Product.pylint, 'pylint', configService, configFileNames); + } + public isEnabled(resource?: Uri): boolean { + const enabled = super.isEnabled(resource); + if (!enabled || this.configService.getSettings(resource).jediEnabled) { + return enabled; + } + // If we're using new LS, then by default Pylint is disabled (unless the user provides a value). + const inspection = this.workspaceService.getConfiguration('python.linting', resource).inspect('pylintEnabled'); + if (!inspection || inspection.globalValue === undefined && inspection.workspaceFolderValue === undefined || inspection.workspaceValue === undefined) { + return false; + } + return enabled; + } +} diff --git a/src/client/linters/linterManager.ts b/src/client/linters/linterManager.ts index 1778736f5e79..478cd31c2ce7 100644 --- a/src/client/linters/linterManager.ts +++ b/src/client/linters/linterManager.ts @@ -7,13 +7,14 @@ import { inject, injectable } from 'inversify'; import { CancellationToken, OutputChannel, TextDocument, Uri } from 'vscode'; +import { IWorkspaceService } from '../common/application/types'; import { IConfigurationService, ILogger, Product } from '../common/types'; import { IServiceContainer } from '../ioc/types'; import { Bandit } from './bandit'; import { Flake8 } from './flake8'; -import { LinterInfo } from './linterInfo'; +import { LinterInfo, PylintLinterInfo } from './linterInfo'; import { MyPy } from './mypy'; import { Pep8 } from './pep8'; import { Prospector } from './prospector'; @@ -36,17 +37,17 @@ class DisabledLinter implements ILinter { @injectable() export class LinterManager implements ILinterManager { - private lintingEnabledSettingName = 'enabled'; private linters: ILinterInfo[]; private configService: IConfigurationService; - private checkedForInstalledLinters: boolean = false; + private checkedForInstalledLinters = new Set(); - constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) { + constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer, + @inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService) { this.configService = serviceContainer.get(IConfigurationService); this.linters = [ new LinterInfo(Product.bandit, 'bandit', this.configService), new LinterInfo(Product.flake8, 'flake8', this.configService), - new LinterInfo(Product.pylint, 'pylint', this.configService, ['.pylintrc', 'pylintrc']), + new PylintLinterInfo(this.configService, this.workspaceService, ['.pylintrc', 'pylintrc']), new LinterInfo(Product.mypy, 'mypy', this.configService), new LinterInfo(Product.pep8, 'pep8', this.configService), new LinterInfo(Product.prospector, 'prospector', this.configService), @@ -70,11 +71,11 @@ export class LinterManager implements ILinterManager { public async isLintingEnabled(silent: boolean, resource?: Uri): Promise { const settings = this.configService.getSettings(resource); const activeLintersPresent = await this.getActiveLinters(silent, resource); - return (settings.linting[this.lintingEnabledSettingName] as boolean) && activeLintersPresent.length > 0; + return settings.linting.enabled && activeLintersPresent.length > 0; } public async enableLintingAsync(enable: boolean, resource?: Uri): Promise { - await this.configService.updateSetting(`linting.${this.lintingEnabledSettingName}`, enable, resource); + await this.configService.updateSetting('linting.enabled', enable, resource); } public async getActiveLinters(silent: boolean, resource?: Uri): Promise { @@ -137,23 +138,21 @@ export class LinterManager implements ILinterManager { throw new Error(error); } - protected async enableUnconfiguredLinters(resource?: Uri): Promise { - // if we've already checked during this session, don't bother again - if (this.checkedForInstalledLinters) { - return false; + protected async enableUnconfiguredLinters(resource?: Uri): Promise { + const settings = this.configService.getSettings(resource); + if (!settings.linting.pylintEnabled || !settings.linting.enabled) { + return; + } + // if we've already checked during this session for the same workspace and python path, then don't bother again. + const workspaceKey = `${this.workspaceService.getWorkspaceFolderIdentifier(resource)}${settings.pythonPath}`; + if (this.checkedForInstalledLinters.has(workspaceKey)) { + return; } - this.checkedForInstalledLinters = true; + this.checkedForInstalledLinters.add(workspaceKey); // 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; + const pylintInfo = this.linters.find(linter => linter.id === 'pylint'); + const activator = this.serviceContainer.get(IAvailableLinterActivator); + await activator.promptIfLinterAvailable(pylintInfo!, resource); } } diff --git a/src/test/linters/lint.args.test.ts b/src/test/linters/lint.args.test.ts index 6f1d0d6ed3e6..43824b00a88a 100644 --- a/src/test/linters/lint.args.test.ts +++ b/src/test/linters/lint.args.test.ts @@ -94,7 +94,7 @@ suite('Linting - Arguments', () => { const platformService = TypeMoq.Mock.ofType(); serviceManager.addSingletonInstance(IPlatformService, platformService.object); - lm = new LinterManager(serviceContainer); + lm = new LinterManager(serviceContainer, workspaceService.object); serviceManager.addSingletonInstance(ILinterManager, lm); document = TypeMoq.Mock.ofType(); }); diff --git a/src/test/linters/lint.commands.test.ts b/src/test/linters/lint.commands.test.ts index d44c548f94ac..04b76f9b8ea5 100644 --- a/src/test/linters/lint.commands.test.ts +++ b/src/test/linters/lint.commands.test.ts @@ -6,7 +6,7 @@ import * as assert from 'assert'; import { Container } from 'inversify'; import * as TypeMoq from 'typemoq'; import { QuickPickOptions } from 'vscode'; -import { IApplicationShell, ICommandManager } from '../../client/common/application/types'; +import { IApplicationShell, ICommandManager, IWorkspaceService } from '../../client/common/application/types'; import { ConfigurationService } from '../../client/common/configuration/service'; import { IConfigurationService, Product } from '../../client/common/types'; import { ServiceContainer } from '../../client/ioc/container'; @@ -49,7 +49,8 @@ suite('Linting - Linter Selector', () => { engine = TypeMoq.Mock.ofType(); serviceManager.addSingletonInstance(ILintingEngine, engine.object); - lm = new LinterManager(serviceContainer); + const workspaceService = TypeMoq.Mock.ofType(); + lm = new LinterManager(serviceContainer, workspaceService.object); serviceManager.addSingletonInstance(ILinterManager, lm); commands = new LinterCommands(serviceContainer); diff --git a/src/test/linters/lint.manager.test.ts b/src/test/linters/lint.manager.test.ts index 7534bd70870c..45532719a5de 100644 --- a/src/test/linters/lint.manager.test.ts +++ b/src/test/linters/lint.manager.test.ts @@ -4,6 +4,8 @@ import * as assert from 'assert'; import { Container } from 'inversify'; +import * as typeMoq from 'typemoq'; +import { IWorkspaceService } from '../../client/common/application/types'; import { ConfigurationService } from '../../client/common/configuration/service'; import { IConfigurationService, ILintingSettings, IPythonSettings, Product } from '../../client/common/types'; import * as EnumEx from '../../client/common/utils/enum'; @@ -31,7 +33,8 @@ suite('Linting - Manager', () => { configService = serviceManager.get(IConfigurationService); settings = configService.getSettings(); - lm = new LinterManager(serviceContainer); + const workspaceService = typeMoq.Mock.ofType(); + lm = new LinterManager(serviceContainer, workspaceService.object); await lm.setActiveLintersAsync([Product.pylint]); await lm.enableLintingAsync(true); diff --git a/src/test/linters/lint.manager.unit.test.ts b/src/test/linters/lint.manager.unit.test.ts index 66208a404628..9b7778b7d7cc 100644 --- a/src/test/linters/lint.manager.unit.test.ts +++ b/src/test/linters/lint.manager.unit.test.ts @@ -6,6 +6,7 @@ import { expect } from 'chai'; import * as TypeMoq from 'typemoq'; import { Uri } from 'vscode'; +import { IWorkspaceService } from '../../client/common/application/types'; import { IConfigurationService, IPythonSettings } from '../../client/common/types'; import { IServiceContainer } from '../../client/ioc/types'; import { LinterManager } from '../../client/linters/linterManager'; @@ -14,9 +15,8 @@ import { LinterManager } from '../../client/linters/linterManager'; class TestLinterManager extends LinterManager { public enableUnconfiguredLintersCallCount: number = 0; - protected async enableUnconfiguredLinters(resource?: Uri): Promise { + protected async enableUnconfiguredLinters(resource?: Uri): Promise { this.enableUnconfiguredLintersCallCount += 1; - return false; } } @@ -33,7 +33,7 @@ function getServiceContainerMockForLinterManagerTests(): TypeMoq.IMock { - + const workspaceService = TypeMoq.Mock.ofType(); test('Linter manager isLintingEnabled checks availability when silent = false.', async () => { // set expectations const expectedCallCount = 1; @@ -43,7 +43,7 @@ suite('Lint Manager Unit Tests', () => { const serviceContainerMock = getServiceContainerMockForLinterManagerTests(); // make the call - const lm = new TestLinterManager(serviceContainerMock.object); + const lm = new TestLinterManager(serviceContainerMock.object, workspaceService.object); await lm.isLintingEnabled(silentFlag); // test expectations @@ -59,7 +59,7 @@ suite('Lint Manager Unit Tests', () => { const serviceContainerMock = getServiceContainerMockForLinterManagerTests(); // make the call - const lm: TestLinterManager = new TestLinterManager(serviceContainerMock.object); + const lm: TestLinterManager = new TestLinterManager(serviceContainerMock.object, workspaceService.object); await lm.isLintingEnabled(silentFlag); // test expectations @@ -75,7 +75,7 @@ suite('Lint Manager Unit Tests', () => { const serviceContainerMock = getServiceContainerMockForLinterManagerTests(); // make the call - const lm: TestLinterManager = new TestLinterManager(serviceContainerMock.object); + const lm: TestLinterManager = new TestLinterManager(serviceContainerMock.object, workspaceService.object); await lm.getActiveLinters(silentFlag); // test expectations @@ -91,7 +91,7 @@ suite('Lint Manager Unit Tests', () => { const serviceContainerMock = getServiceContainerMockForLinterManagerTests(); // make the call - const lm: TestLinterManager = new TestLinterManager(serviceContainerMock.object); + const lm: TestLinterManager = new TestLinterManager(serviceContainerMock.object, workspaceService.object); await lm.getActiveLinters(silentFlag); // test expectations diff --git a/src/test/linters/lint.provider.test.ts b/src/test/linters/lint.provider.test.ts index 4b3601f32318..a78232b54491 100644 --- a/src/test/linters/lint.provider.test.ts +++ b/src/test/linters/lint.provider.test.ts @@ -83,7 +83,7 @@ suite('Linting - Provider', () => { serviceManager.addSingletonInstance(IWorkspaceService, workspaceService.object); serviceManager.add(IAvailableLinterActivator, AvailableLinterActivator); - lm = new LinterManager(serviceContainer); + lm = new LinterManager(serviceContainer, workspaceService.object); serviceManager.addSingletonInstance(ILinterManager, lm); emitter = new vscode.EventEmitter(); document = TypeMoq.Mock.ofType(); diff --git a/src/test/linters/lint.test.ts b/src/test/linters/lint.test.ts index 5fe3ce7d6e6f..747719f11042 100644 --- a/src/test/linters/lint.test.ts +++ b/src/test/linters/lint.test.ts @@ -7,6 +7,7 @@ import * as fs from 'fs-extra'; import * as path from 'path'; import { CancellationTokenSource, ConfigurationTarget, DiagnosticCollection, Uri, window, workspace } from 'vscode'; import { ICommandManager } from '../../client/common/application/types'; +import { WorkspaceService } from '../../client/common/application/workspace'; import { STANDARD_OUTPUT_CHANNEL } from '../../client/common/constants'; import { Product } from '../../client/common/installer/productInstaller'; import { CTagsProductPathService, FormatterProductPathService, LinterProductPathService, RefactoringLibraryProductPathService, TestFrameworkProductPathService } from '../../client/common/installer/productPath'; @@ -127,7 +128,7 @@ suite('Linting - General Tests', () => { ioc.registerLinterTypes(); ioc.registerVariableTypes(); ioc.registerPlatformTypes(); - linterManager = new LinterManager(ioc.serviceContainer); + linterManager = new LinterManager(ioc.serviceContainer, new WorkspaceService()); configService = ioc.serviceContainer.get(IConfigurationService); ioc.serviceManager.addSingletonInstance(IProductService, new ProductService()); ioc.serviceManager.addSingleton(IProductPathService, CTagsProductPathService, ProductType.WorkspaceSymbols); diff --git a/src/test/linters/pylint.test.ts b/src/test/linters/pylint.test.ts index 66bc7fdb956d..4c59626ad324 100644 --- a/src/test/linters/pylint.test.ts +++ b/src/test/linters/pylint.test.ts @@ -54,7 +54,7 @@ suite('Linting - Pylint', () => { config = TypeMoq.Mock.ofType(); serviceManager.addSingletonInstance(IConfigurationService, config.object); - const linterManager = new LinterManager(serviceContainer); + const linterManager = new LinterManager(serviceContainer, workspace.object); serviceManager.addSingletonInstance(ILinterManager, linterManager); const logger = TypeMoq.Mock.ofType(); serviceManager.addSingletonInstance(ILogger, logger.object); From e248a30c6c89cc40d6967d07d9d6578f4749af1d Mon Sep 17 00:00:00 2001 From: Brett Cannon Date: Tue, 6 Nov 2018 15:33:21 -0800 Subject: [PATCH 2/2] oh well ok Co-Authored-By: DonJayamanne --- src/client/linters/linterManager.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/linters/linterManager.ts b/src/client/linters/linterManager.ts index 478cd31c2ce7..af82baff3c62 100644 --- a/src/client/linters/linterManager.ts +++ b/src/client/linters/linterManager.ts @@ -143,7 +143,7 @@ export class LinterManager implements ILinterManager { if (!settings.linting.pylintEnabled || !settings.linting.enabled) { return; } - // if we've already checked during this session for the same workspace and python path, then don't bother again. + // If we've already checked during this session for the same workspace and Python path, then don't bother again. const workspaceKey = `${this.workspaceService.getWorkspaceFolderIdentifier(resource)}${settings.pythonPath}`; if (this.checkedForInstalledLinters.has(workspaceKey)) { return;