Fixes to pylint checks (#3225)

For #974

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.
      *

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<ConfigurationChangeEvent> {
-        return vscode.workspace.onDidChangeConfiguration;
+    public get onDidChangeConfiguration(): Event<ConfigurationChangeEvent> {
+        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<vscode.WorkspaceFoldersChangeEvent> {
-        return vscode.workspace.onDidChangeWorkspaceFolders;
+    public get onDidChangeWorkspaceFolders(): Event<WorkspaceFoldersChangeEvent> {
+        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<vscode.Uri[]> {
-        return vscode.workspace.findFiles(include, exclude, maxResults, token);
+    public findFiles(include: GlobPattern, exclude?: GlobPattern, maxResults?: number, token?: CancellationToken): Thenable<Uri[]> {
+        return workspace.findFiles(include, exclude, maxResults, token);
     }
-    public get onDidSaveTextDocument(): vscode.Event<vscode.TextDocument> {
-        return vscode.workspace.onDidSaveTextDocument;
+    public getWorkspaceFolderIdentifier(resource: Uri): string {
+        const workspaceFolder = resource ? workspace.getWorkspaceFolder(resource) : undefined;
+        return workspaceFolder ? workspaceFolder.uri.fsPath : '';
     }
 }

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<boolean>('pylintEnabled');
+        if (!inspection || inspection.globalValue === undefined && inspection.workspaceFolderValue === undefined || inspection.workspaceValue === undefined) {
+            return false;
+        }
+        return enabled;
+    }
+}

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<string>();
 
-    constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) {
+    constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer,
+        @inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService) {
         this.configService = serviceContainer.get<IConfigurationService>(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<boolean> {
         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<void> {
-        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<ILinterInfo[]> {
@@ -137,23 +138,21 @@ export class LinterManager implements ILinterManager {
         throw new Error(error);
     }
 
-    protected async enableUnconfiguredLinters(resource?: Uri): Promise<boolean> {
-        // if we've already checked during this session, don't bother again
-        if (this.checkedForInstalledLinters) {
-            return false;
+    protected async enableUnconfiguredLinters(resource?: Uri): Promise<void> {
+        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>(IAvailableLinterActivator);
-            return activator.promptIfLinterAvailable(pylintInfo, resource);
-        }
-        return false;
+        const pylintInfo = this.linters.find(linter => linter.id === 'pylint');
+        const activator = this.serviceContainer.get<IAvailableLinterActivator>(IAvailableLinterActivator);
+        await activator.promptIfLinterAvailable(pylintInfo!, resource);
     }
 }

src/test/linters/lint.args.test.ts

@@ -94,7 +94,7 @@ suite('Linting - Arguments', () => {
                     const platformService = TypeMoq.Mock.ofType<IPlatformService>();
                     serviceManager.addSingletonInstance<IPlatformService>(IPlatformService, platformService.object);
 
-                    lm = new LinterManager(serviceContainer);
+                    lm = new LinterManager(serviceContainer, workspaceService.object);
                     serviceManager.addSingletonInstance<ILinterManager>(ILinterManager, lm);
                     document = TypeMoq.Mock.ofType<TextDocument>();
                 });

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<ILintingEngine>();
         serviceManager.addSingletonInstance<ILintingEngine>(ILintingEngine, engine.object);
 
-        lm = new LinterManager(serviceContainer);
+        const workspaceService = TypeMoq.Mock.ofType<IWorkspaceService>();
+        lm = new LinterManager(serviceContainer, workspaceService.object);
         serviceManager.addSingletonInstance<ILinterManager>(ILinterManager, lm);
 
         commands = new LinterCommands(serviceContainer);

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>(IConfigurationService);
 
         settings = configService.getSettings();
-        lm = new LinterManager(serviceContainer);
+        const workspaceService = typeMoq.Mock.ofType<IWorkspaceService>();
+        lm = new LinterManager(serviceContainer, workspaceService.object);
 
         await lm.setActiveLintersAsync([Product.pylint]);
         await lm.enableLintingAsync(true);

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<boolean> {
+    protected async enableUnconfiguredLinters(resource?: Uri): Promise<void> {
         this.enableUnconfiguredLintersCallCount += 1;
-        return false;
     }
 }
 
@@ -33,7 +33,7 @@ function getServiceContainerMockForLinterManagerTests(): TypeMoq.IMock<IServiceC
 
 // tslint:disable-next-line:max-func-body-length
 suite('Lint Manager Unit Tests', () => {
-
+    const workspaceService = TypeMoq.Mock.ofType<IWorkspaceService>();
     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

src/test/linters/lint.provider.test.ts

@@ -83,7 +83,7 @@ suite('Linting - Provider', () => {
         serviceManager.addSingletonInstance<IWorkspaceService>(IWorkspaceService, workspaceService.object);
         serviceManager.add(IAvailableLinterActivator, AvailableLinterActivator);
 
-        lm = new LinterManager(serviceContainer);
+        lm = new LinterManager(serviceContainer, workspaceService.object);
         serviceManager.addSingletonInstance<ILinterManager>(ILinterManager, lm);
         emitter = new vscode.EventEmitter<vscode.TextDocument>();
         document = TypeMoq.Mock.ofType<vscode.TextDocument>();

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>(IConfigurationService);
         ioc.serviceManager.addSingletonInstance<IProductService>(IProductService, new ProductService());
         ioc.serviceManager.addSingleton<IProductPathService>(IProductPathService, CTagsProductPathService, ProductType.WorkspaceSymbols);

src/test/linters/pylint.test.ts

@@ -54,7 +54,7 @@ suite('Linting - Pylint', () => {
 
         config = TypeMoq.Mock.ofType<IConfigurationService>();
         serviceManager.addSingletonInstance<IConfigurationService>(IConfigurationService, config.object);
-        const linterManager = new LinterManager(serviceContainer);
+        const linterManager = new LinterManager(serviceContainer, workspace.object);
         serviceManager.addSingletonInstance<ILinterManager>(ILinterManager, linterManager);
         const logger = TypeMoq.Mock.ofType<ILogger>();
         serviceManager.addSingletonInstance<ILogger>(ILogger, logger.object);