Skip to content

Commit e3b047d

Browse files
DonJayamanneDonJayamanne
authored
Fixes to pylint checks (#3225)
For #974
1 parent 044f768 commit e3b047d

11 files changed

Lines changed: 89 additions & 59 deletions

File tree

‎src/client/common/application/types.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -556,6 +556,13 @@ export interface IWorkspaceService {
556556
*/
557557
getWorkspaceFolder(uri: Uri): WorkspaceFolder | undefined;
558558

559+
/**
560+
* Generate a key that's unique to the workspace folder (could be fsPath).
561+
* @param {(Uri | undefined)} resource
562+
* @returns {string}
563+
* @memberof IWorkspaceService
564+
*/
565+
getWorkspaceFolderIdentifier(resource: Uri | undefined): string;
559566
/**
560567
* Returns a path that is relative to the workspace folder or folders.
561568
*

‎src/client/common/application/workspace.ts‎

Lines changed: 22 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -2,43 +2,43 @@
22
// Licensed under the MIT License.
33

44
import { injectable } from 'inversify';
5-
import * as vscode from 'vscode';
6-
import { ConfigurationChangeEvent } from 'vscode';
5+
import { CancellationToken, ConfigurationChangeEvent, Event, FileSystemWatcher, GlobPattern, Uri, workspace, WorkspaceConfiguration, WorkspaceFolder, WorkspaceFoldersChangeEvent } from 'vscode';
76
import { IWorkspaceService } from './types';
87

98
@injectable()
109
export class WorkspaceService implements IWorkspaceService {
11-
public get onDidChangeConfiguration(): vscode.Event<ConfigurationChangeEvent> {
12-
return vscode.workspace.onDidChangeConfiguration;
10+
public get onDidChangeConfiguration(): Event<ConfigurationChangeEvent> {
11+
return workspace.onDidChangeConfiguration;
1312
}
1413
public get rootPath(): string | undefined {
15-
return Array.isArray(vscode.workspace.workspaceFolders) ? vscode.workspace.workspaceFolders[0].uri.fsPath : undefined;
14+
return Array.isArray(workspace.workspaceFolders) ? workspace.workspaceFolders[0].uri.fsPath : undefined;
1615
}
17-
public get workspaceFolders(): vscode.WorkspaceFolder[] | undefined {
18-
return vscode.workspace.workspaceFolders;
16+
public get workspaceFolders(): WorkspaceFolder[] | undefined {
17+
return workspace.workspaceFolders;
1918
}
20-
public get onDidChangeWorkspaceFolders(): vscode.Event<vscode.WorkspaceFoldersChangeEvent> {
21-
return vscode.workspace.onDidChangeWorkspaceFolders;
19+
public get onDidChangeWorkspaceFolders(): Event<WorkspaceFoldersChangeEvent> {
20+
return workspace.onDidChangeWorkspaceFolders;
2221
}
2322
public get hasWorkspaceFolders() {
24-
return Array.isArray(vscode.workspace.workspaceFolders) && vscode.workspace.workspaceFolders.length > 0;
23+
return Array.isArray(workspace.workspaceFolders) && workspace.workspaceFolders.length > 0;
2524
}
26-
public getConfiguration(section?: string, resource?: vscode.Uri): vscode.WorkspaceConfiguration {
27-
return vscode.workspace.getConfiguration(section, resource);
25+
public getConfiguration(section?: string, resource?: Uri): WorkspaceConfiguration {
26+
return workspace.getConfiguration(section, resource);
2827
}
29-
public getWorkspaceFolder(uri: vscode.Uri): vscode.WorkspaceFolder | undefined {
30-
return vscode.workspace.getWorkspaceFolder(uri);
28+
public getWorkspaceFolder(uri: Uri): WorkspaceFolder | undefined {
29+
return workspace.getWorkspaceFolder(uri);
3130
}
32-
public asRelativePath(pathOrUri: string | vscode.Uri, includeWorkspaceFolder?: boolean): string {
33-
return vscode.workspace.asRelativePath(pathOrUri, includeWorkspaceFolder);
31+
public asRelativePath(pathOrUri: string | Uri, includeWorkspaceFolder?: boolean): string {
32+
return workspace.asRelativePath(pathOrUri, includeWorkspaceFolder);
3433
}
35-
public createFileSystemWatcher(globPattern: vscode.GlobPattern, ignoreCreateEvents?: boolean, ignoreChangeEvents?: boolean, ignoreDeleteEvents?: boolean): vscode.FileSystemWatcher {
36-
return vscode.workspace.createFileSystemWatcher(globPattern, ignoreChangeEvents, ignoreChangeEvents, ignoreDeleteEvents);
34+
public createFileSystemWatcher(globPattern: GlobPattern, ignoreCreateEvents?: boolean, ignoreChangeEvents?: boolean, ignoreDeleteEvents?: boolean): FileSystemWatcher {
35+
return workspace.createFileSystemWatcher(globPattern, ignoreChangeEvents, ignoreChangeEvents, ignoreDeleteEvents);
3736
}
38-
public findFiles(include: vscode.GlobPattern, exclude?: vscode.GlobPattern, maxResults?: number, token?: vscode.CancellationToken): Thenable<vscode.Uri[]> {
39-
return vscode.workspace.findFiles(include, exclude, maxResults, token);
37+
public findFiles(include: GlobPattern, exclude?: GlobPattern, maxResults?: number, token?: CancellationToken): Thenable<Uri[]> {
38+
return workspace.findFiles(include, exclude, maxResults, token);
4039
}
41-
public get onDidSaveTextDocument(): vscode.Event<vscode.TextDocument> {
42-
return vscode.workspace.onDidSaveTextDocument;
40+
public getWorkspaceFolderIdentifier(resource: Uri): string {
41+
const workspaceFolder = resource ? workspace.getWorkspaceFolder(resource) : undefined;
42+
return workspaceFolder ? workspaceFolder.uri.fsPath : '';
4343
}
4444
}

‎src/client/linters/linterInfo.ts‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33

44
import * as path from 'path';
55
import { Uri } from 'vscode';
6+
import { IWorkspaceService } from '../common/application/types';
67
import { ExecutionInfo, IConfigurationService, Product } from '../common/types';
78
import { ILinterInfo, LinterId } from './types';
89

@@ -11,7 +12,7 @@ export class LinterInfo implements ILinterInfo {
1112
private _product: Product;
1213
private _configFileNames: string[];
1314

14-
constructor(product: Product, id: LinterId, private configService: IConfigurationService, configFileNames: string[] = []) {
15+
constructor(product: Product, id: LinterId, protected configService: IConfigurationService, configFileNames: string[] = []) {
1516
this._product = product;
1617
this._id = id;
1718
this._configFileNames = configFileNames;
@@ -67,3 +68,21 @@ export class LinterInfo implements ILinterInfo {
6768
return { execPath, moduleName, args, product: this.product };
6869
}
6970
}
71+
72+
export class PylintLinterInfo extends LinterInfo {
73+
constructor(configService: IConfigurationService, private readonly workspaceService: IWorkspaceService, configFileNames: string[] = []) {
74+
super(Product.pylint, 'pylint', configService, configFileNames);
75+
}
76+
public isEnabled(resource?: Uri): boolean {
77+
const enabled = super.isEnabled(resource);
78+
if (!enabled || this.configService.getSettings(resource).jediEnabled) {
79+
return enabled;
80+
}
81+
// If we're using new LS, then by default Pylint is disabled (unless the user provides a value).
82+
const inspection = this.workspaceService.getConfiguration('python.linting', resource).inspect<boolean>('pylintEnabled');
83+
if (!inspection || inspection.globalValue === undefined && inspection.workspaceFolderValue === undefined || inspection.workspaceValue === undefined) {
84+
return false;
85+
}
86+
return enabled;
87+
}
88+
}

‎src/client/linters/linterManager.ts‎

Lines changed: 21 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,14 @@ import { inject, injectable } from 'inversify';
77
import {
88
CancellationToken, OutputChannel, TextDocument, Uri
99
} from 'vscode';
10+
import { IWorkspaceService } from '../common/application/types';
1011
import {
1112
IConfigurationService, ILogger, Product
1213
} from '../common/types';
1314
import { IServiceContainer } from '../ioc/types';
1415
import { Bandit } from './bandit';
1516
import { Flake8 } from './flake8';
16-
import { LinterInfo } from './linterInfo';
17+
import { LinterInfo, PylintLinterInfo } from './linterInfo';
1718
import { MyPy } from './mypy';
1819
import { Pep8 } from './pep8';
1920
import { Prospector } from './prospector';
@@ -36,17 +37,17 @@ class DisabledLinter implements ILinter {
3637

3738
@injectable()
3839
export class LinterManager implements ILinterManager {
39-
private lintingEnabledSettingName = 'enabled';
4040
private linters: ILinterInfo[];
4141
private configService: IConfigurationService;
42-
private checkedForInstalledLinters: boolean = false;
42+
private checkedForInstalledLinters = new Set<string>();
4343

44-
constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer) {
44+
constructor(@inject(IServiceContainer) private serviceContainer: IServiceContainer,
45+
@inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService) {
4546
this.configService = serviceContainer.get<IConfigurationService>(IConfigurationService);
4647
this.linters = [
4748
new LinterInfo(Product.bandit, 'bandit', this.configService),
4849
new LinterInfo(Product.flake8, 'flake8', this.configService),
49-
new LinterInfo(Product.pylint, 'pylint', this.configService, ['.pylintrc', 'pylintrc']),
50+
new PylintLinterInfo(this.configService, this.workspaceService, ['.pylintrc', 'pylintrc']),
5051
new LinterInfo(Product.mypy, 'mypy', this.configService),
5152
new LinterInfo(Product.pep8, 'pep8', this.configService),
5253
new LinterInfo(Product.prospector, 'prospector', this.configService),
@@ -70,11 +71,11 @@ export class LinterManager implements ILinterManager {
7071
public async isLintingEnabled(silent: boolean, resource?: Uri): Promise<boolean> {
7172
const settings = this.configService.getSettings(resource);
7273
const activeLintersPresent = await this.getActiveLinters(silent, resource);
73-
return (settings.linting[this.lintingEnabledSettingName] as boolean) && activeLintersPresent.length > 0;
74+
return settings.linting.enabled && activeLintersPresent.length > 0;
7475
}
7576

7677
public async enableLintingAsync(enable: boolean, resource?: Uri): Promise<void> {
77-
await this.configService.updateSetting(`linting.${this.lintingEnabledSettingName}`, enable, resource);
78+
await this.configService.updateSetting('linting.enabled', enable, resource);
7879
}
7980

8081
public async getActiveLinters(silent: boolean, resource?: Uri): Promise<ILinterInfo[]> {
@@ -137,23 +138,21 @@ export class LinterManager implements ILinterManager {
137138
throw new Error(error);
138139
}
139140

140-
protected async enableUnconfiguredLinters(resource?: Uri): Promise<boolean> {
141-
// if we've already checked during this session, don't bother again
142-
if (this.checkedForInstalledLinters) {
143-
return false;
141+
protected async enableUnconfiguredLinters(resource?: Uri): Promise<void> {
142+
const settings = this.configService.getSettings(resource);
143+
if (!settings.linting.pylintEnabled || !settings.linting.enabled) {
144+
return;
145+
}
146+
// If we've already checked during this session for the same workspace and Python path, then don't bother again.
147+
const workspaceKey = `${this.workspaceService.getWorkspaceFolderIdentifier(resource)}${settings.pythonPath}`;
148+
if (this.checkedForInstalledLinters.has(workspaceKey)) {
149+
return;
144150
}
145-
this.checkedForInstalledLinters = true;
151+
this.checkedForInstalledLinters.add(workspaceKey);
146152

147153
// only check & ask the user if they'd like to enable pylint
148-
const pylintInfo = this.linters.find(
149-
(linter: ILinterInfo) => linter.id === 'pylint'
150-
);
151-
152-
// If linting is disabled, don't bother checking further.
153-
if (pylintInfo && await this.isLintingEnabled(true, resource)) {
154-
const activator = this.serviceContainer.get<IAvailableLinterActivator>(IAvailableLinterActivator);
155-
return activator.promptIfLinterAvailable(pylintInfo, resource);
156-
}
157-
return false;
154+
const pylintInfo = this.linters.find(linter => linter.id === 'pylint');
155+
const activator = this.serviceContainer.get<IAvailableLinterActivator>(IAvailableLinterActivator);
156+
await activator.promptIfLinterAvailable(pylintInfo!, resource);
158157
}
159158
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ suite('Linting - Arguments', () => {
9494
const platformService = TypeMoq.Mock.ofType<IPlatformService>();
9595
serviceManager.addSingletonInstance<IPlatformService>(IPlatformService, platformService.object);
9696

97-
lm = new LinterManager(serviceContainer);
97+
lm = new LinterManager(serviceContainer, workspaceService.object);
9898
serviceManager.addSingletonInstance<ILinterManager>(ILinterManager, lm);
9999
document = TypeMoq.Mock.ofType<TextDocument>();
100100
});

‎src/test/linters/lint.commands.test.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import * as assert from 'assert';
66
import { Container } from 'inversify';
77
import * as TypeMoq from 'typemoq';
88
import { QuickPickOptions } from 'vscode';
9-
import { IApplicationShell, ICommandManager } from '../../client/common/application/types';
9+
import { IApplicationShell, ICommandManager, IWorkspaceService } from '../../client/common/application/types';
1010
import { ConfigurationService } from '../../client/common/configuration/service';
1111
import { IConfigurationService, Product } from '../../client/common/types';
1212
import { ServiceContainer } from '../../client/ioc/container';
@@ -49,7 +49,8 @@ suite('Linting - Linter Selector', () => {
4949
engine = TypeMoq.Mock.ofType<ILintingEngine>();
5050
serviceManager.addSingletonInstance<ILintingEngine>(ILintingEngine, engine.object);
5151

52-
lm = new LinterManager(serviceContainer);
52+
const workspaceService = TypeMoq.Mock.ofType<IWorkspaceService>();
53+
lm = new LinterManager(serviceContainer, workspaceService.object);
5354
serviceManager.addSingletonInstance<ILinterManager>(ILinterManager, lm);
5455

5556
commands = new LinterCommands(serviceContainer);

‎src/test/linters/lint.manager.test.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44

55
import * as assert from 'assert';
66
import { Container } from 'inversify';
7+
import * as typeMoq from 'typemoq';
8+
import { IWorkspaceService } from '../../client/common/application/types';
79
import { ConfigurationService } from '../../client/common/configuration/service';
810
import { IConfigurationService, ILintingSettings, IPythonSettings, Product } from '../../client/common/types';
911
import * as EnumEx from '../../client/common/utils/enum';
@@ -31,7 +33,8 @@ suite('Linting - Manager', () => {
3133
configService = serviceManager.get<IConfigurationService>(IConfigurationService);
3234

3335
settings = configService.getSettings();
34-
lm = new LinterManager(serviceContainer);
36+
const workspaceService = typeMoq.Mock.ofType<IWorkspaceService>();
37+
lm = new LinterManager(serviceContainer, workspaceService.object);
3538

3639
await lm.setActiveLintersAsync([Product.pylint]);
3740
await lm.enableLintingAsync(true);

‎src/test/linters/lint.manager.unit.test.ts‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
import { expect } from 'chai';
77
import * as TypeMoq from 'typemoq';
88
import { Uri } from 'vscode';
9+
import { IWorkspaceService } from '../../client/common/application/types';
910
import { IConfigurationService, IPythonSettings } from '../../client/common/types';
1011
import { IServiceContainer } from '../../client/ioc/types';
1112
import { LinterManager } from '../../client/linters/linterManager';
@@ -14,9 +15,8 @@ import { LinterManager } from '../../client/linters/linterManager';
1415
class TestLinterManager extends LinterManager {
1516
public enableUnconfiguredLintersCallCount: number = 0;
1617

17-
protected async enableUnconfiguredLinters(resource?: Uri): Promise<boolean> {
18+
protected async enableUnconfiguredLinters(resource?: Uri): Promise<void> {
1819
this.enableUnconfiguredLintersCallCount += 1;
19-
return false;
2020
}
2121
}
2222

@@ -33,7 +33,7 @@ function getServiceContainerMockForLinterManagerTests(): TypeMoq.IMock<IServiceC
3333

3434
// tslint:disable-next-line:max-func-body-length
3535
suite('Lint Manager Unit Tests', () => {
36-
36+
const workspaceService = TypeMoq.Mock.ofType<IWorkspaceService>();
3737
test('Linter manager isLintingEnabled checks availability when silent = false.', async () => {
3838
// set expectations
3939
const expectedCallCount = 1;
@@ -43,7 +43,7 @@ suite('Lint Manager Unit Tests', () => {
4343
const serviceContainerMock = getServiceContainerMockForLinterManagerTests();
4444

4545
// make the call
46-
const lm = new TestLinterManager(serviceContainerMock.object);
46+
const lm = new TestLinterManager(serviceContainerMock.object, workspaceService.object);
4747
await lm.isLintingEnabled(silentFlag);
4848

4949
// test expectations
@@ -59,7 +59,7 @@ suite('Lint Manager Unit Tests', () => {
5959
const serviceContainerMock = getServiceContainerMockForLinterManagerTests();
6060

6161
// make the call
62-
const lm: TestLinterManager = new TestLinterManager(serviceContainerMock.object);
62+
const lm: TestLinterManager = new TestLinterManager(serviceContainerMock.object, workspaceService.object);
6363
await lm.isLintingEnabled(silentFlag);
6464

6565
// test expectations
@@ -75,7 +75,7 @@ suite('Lint Manager Unit Tests', () => {
7575
const serviceContainerMock = getServiceContainerMockForLinterManagerTests();
7676

7777
// make the call
78-
const lm: TestLinterManager = new TestLinterManager(serviceContainerMock.object);
78+
const lm: TestLinterManager = new TestLinterManager(serviceContainerMock.object, workspaceService.object);
7979
await lm.getActiveLinters(silentFlag);
8080

8181
// test expectations
@@ -91,7 +91,7 @@ suite('Lint Manager Unit Tests', () => {
9191
const serviceContainerMock = getServiceContainerMockForLinterManagerTests();
9292

9393
// make the call
94-
const lm: TestLinterManager = new TestLinterManager(serviceContainerMock.object);
94+
const lm: TestLinterManager = new TestLinterManager(serviceContainerMock.object, workspaceService.object);
9595
await lm.getActiveLinters(silentFlag);
9696

9797
// test expectations

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ suite('Linting - Provider', () => {
8383
serviceManager.addSingletonInstance<IWorkspaceService>(IWorkspaceService, workspaceService.object);
8484
serviceManager.add(IAvailableLinterActivator, AvailableLinterActivator);
8585

86-
lm = new LinterManager(serviceContainer);
86+
lm = new LinterManager(serviceContainer, workspaceService.object);
8787
serviceManager.addSingletonInstance<ILinterManager>(ILinterManager, lm);
8888
emitter = new vscode.EventEmitter<vscode.TextDocument>();
8989
document = TypeMoq.Mock.ofType<vscode.TextDocument>();

‎src/test/linters/lint.test.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import * as fs from 'fs-extra';
77
import * as path from 'path';
88
import { CancellationTokenSource, ConfigurationTarget, DiagnosticCollection, Uri, window, workspace } from 'vscode';
99
import { ICommandManager } from '../../client/common/application/types';
10+
import { WorkspaceService } from '../../client/common/application/workspace';
1011
import { STANDARD_OUTPUT_CHANNEL } from '../../client/common/constants';
1112
import { Product } from '../../client/common/installer/productInstaller';
1213
import { CTagsProductPathService, FormatterProductPathService, LinterProductPathService, RefactoringLibraryProductPathService, TestFrameworkProductPathService } from '../../client/common/installer/productPath';
@@ -127,7 +128,7 @@ suite('Linting - General Tests', () => {
127128
ioc.registerLinterTypes();
128129
ioc.registerVariableTypes();
129130
ioc.registerPlatformTypes();
130-
linterManager = new LinterManager(ioc.serviceContainer);
131+
linterManager = new LinterManager(ioc.serviceContainer, new WorkspaceService());
131132
configService = ioc.serviceContainer.get<IConfigurationService>(IConfigurationService);
132133
ioc.serviceManager.addSingletonInstance<IProductService>(IProductService, new ProductService());
133134
ioc.serviceManager.addSingleton<IProductPathService>(IProductPathService, CTagsProductPathService, ProductType.WorkspaceSymbols);

0 commit comments

Comments
 (0)