Skip to content

Ensure pylint is available if "pylintrc" file is present #5369

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
karrtikr merged 1 commit into from
Apr 19, 2019
Merged

Ensure pylint is available if "pylintrc" file is present #5369

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions src/client/linters/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,3 @@ export const LINTERID_BY_PRODUCT = new Map<Product, LinterId>([
[Product.pydocstyle, 'pydocstyle'],
[Product.pylama, 'pylama']
]);

export const PYLINT_CONFIG = '.pylintrc';
23 changes: 11 additions & 12 deletions src/client/linters/linterAvailability.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,12 @@ import * as path from 'path';
import { Uri } from 'vscode';
import { IApplicationShell, IWorkspaceService } from '../common/application/types';
import '../common/extensions';
import { traceError } from '../common/logger';
import { traceDecorators } from '../common/logger';
import { IFileSystem } from '../common/platform/types';
import { IConfigurationService, IPersistentStateFactory, Product } from '../common/types';
import { IConfigurationService, IPersistentStateFactory } from '../common/types';
import { Common, Linters } from '../common/utils/localize';
import { sendTelemetryEvent } from '../telemetry';
import { EventName } from '../telemetry/constants';
import { PYLINT_CONFIG } from './constants';
import { IAvailableLinterActivator, ILinterInfo } from './types';

const doNotDisplayPromptStateKey = 'MESSAGE_KEY_FOR_CONFIGURE_AVAILABLE_LINTER_PROMPT';
Expand Down Expand Up @@ -50,7 +49,7 @@ export class AvailableLinterActivator implements IAvailableLinterActivator {
}

// Is the linter available in the current workspace?
if (await this.isLinterAvailable(linterInfo.product, resource)) {
if (await this.isLinterAvailable(linterInfo, resource)) {

// great, it is - ask the user if they'd like to enable it.
return this.promptToConfigureAvailableLinter(linterInfo);
Expand Down Expand Up @@ -96,18 +95,18 @@ export class AvailableLinterActivator implements IAvailableLinterActivator {
* @param linterProduct Linter to check in the current workspace environment.
* @param resource Context information for workspace.
*/
public async isLinterAvailable(linterProduct: Product, resource?: Uri): Promise<boolean | undefined> {
@traceDecorators.error('Failed to discover if linter pylint is available')
public async isLinterAvailable(linterInfo: ILinterInfo, resource?: Uri): Promise<boolean | undefined> {
if (!this.workspaceService.hasWorkspaceFolders) {
return false;
}
const workspaceFolder = resource ? this.workspaceService.getWorkspaceFolder(resource)! : this.workspaceService.workspaceFolders![0];
const filePath = path.join(workspaceFolder.uri.fsPath, PYLINT_CONFIG);
return this.fs.fileExists(filePath)
.catch((reason) => {
// report and continue, assume the linter is unavailable.
traceError(`[WARNING]: Failed to discover if linter ${linterProduct} is installed.`, reason);
return false;
});
let isAvailable = false;
for (const configName of linterInfo.configFileNames) {
const configPath = path.join(workspaceFolder.uri.fsPath, configName);
isAvailable = isAvailable || await this.fs.fileExists(configPath);
}
return isAvailable;
}

/**
Expand Down
40 changes: 5 additions & 35 deletions src/test/linters/linter.availability.unit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ suite('Linter Availability Provider tests', () => {
.verifiable(TypeMoq.Times.once());
fsMock.setup(fs => fs.fileExists(TypeMoq.It.isAny()))
.returns(async () => options.linterIsInstalled)
.verifiable(TypeMoq.Times.once());
.verifiable(TypeMoq.Times.atLeastOnce());

setupConfigurationServiceForJediSettingsTest(options.jediEnabledValue, configServiceMock);
setupWorkspaceMockForLinterConfiguredTests(
Expand Down Expand Up @@ -413,7 +413,7 @@ suite('Linter Availability Provider tests', () => {

// perform test
const availabilityProvider = new AvailableLinterActivator(appShellMock.object, fsMock.object, workspaceServiceMock.object, configServiceMock.object, factoryMock.object);
const result = await availabilityProvider.isLinterAvailable(linterInfo.product);
const result = await availabilityProvider.isLinterAvailable(linterInfo);

expect(result).to.equal(expectedResult, 'Expected promptToConfigureAvailableLinter to return true because the configuration was updated.');
fsMock.verifyAll();
Expand All @@ -431,37 +431,7 @@ suite('Linter Availability Provider tests', () => {

// perform test
const availabilityProvider = new AvailableLinterActivator(appShellMock.object, fsMock.object, workspaceServiceMock.object, configServiceMock.object, factoryMock.object);
const result = await availabilityProvider.isLinterAvailable(linterInfo.product);

expect(result).to.equal(expectedResult, 'Expected promptToConfigureAvailableLinter to return true because the configuration was updated.');
fsMock.verifyAll();
workspaceServiceMock.verifyAll();
});

test('Discovery of linter is available in the environment returns false when it fails', async () => {
// set expectations
const expectedResult = false;

// arrange
const [appShellMock, fsMock, workspaceServiceMock, configServiceMock, factoryMock, linterInfo] = getDependenciesForAvailabilityTests();
const workspaceFolder = { uri: Uri.parse('full/path/to/workspace'), name: '', index: 0 };
workspaceServiceMock
.setup(c => c.hasWorkspaceFolders)
.returns(() => true)
.verifiable(TypeMoq.Times.once());
workspaceServiceMock
.setup(c => c.workspaceFolders)
.returns(() => [workspaceFolder]);
workspaceServiceMock
.setup(c => c.getWorkspaceFolder(TypeMoq.It.isAny()))
.returns(() => workspaceFolder);
fsMock.setup(fs => fs.fileExists(TypeMoq.It.isAny()))
.returns(async () => Promise.reject('error testfail'))
.verifiable(TypeMoq.Times.once());

// perform test
const availabilityProvider = new AvailableLinterActivator(appShellMock.object, fsMock.object, workspaceServiceMock.object, configServiceMock.object, factoryMock.object);
const result = await availabilityProvider.isLinterAvailable(linterInfo.product);
const result = await availabilityProvider.isLinterAvailable(linterInfo);

expect(result).to.equal(expectedResult, 'Expected promptToConfigureAvailableLinter to return true because the configuration was updated.');
fsMock.verifyAll();
Expand Down Expand Up @@ -532,8 +502,8 @@ function setupInstallerForAvailabilityTest(_linterInfo: LinterInfo, linterIsInst
.setup(c => c.getWorkspaceFolder(TypeMoq.It.isAny()))
.returns(() => workspaceFolder);
fsMock.setup(fs => fs.fileExists(TypeMoq.It.isAny()))
.returns(async () => linterIsInstalled)
.verifiable(TypeMoq.Times.once());
.returns(() => Promise.resolve(linterIsInstalled))
.verifiable(TypeMoq.Times.atLeastOnce());

return fsMock;
}
Expand Down