Pylint not enabled by default, can be easily enabled for workspaces that have it available. (#2913)

Prepare Pylint to no longer be enabled by default. Fix for #974 - Pylint is easily enabled for workspaces that have it available - Relocate logic that checks for and enables available linter.

Author: d3r3kk

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.

package.json

@@ -1690,4 +1690,4 @@
  "publisherDisplayName": "Microsoft",
  "publisherId": "998b010b-e2af-44a5-a6cd-0b5fd3b9b6f8"
  }
-}
+}

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<ILintMessage[]> {
  this._pythonSettings = this.configService.getSettings(document.uri);
  return this.runLinter(document, cancellation);

src/client/linters/linterAvailability.ts

@@ -0,0 +1,127 @@
+// 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 { traceError } from '../common/logger';
+import { IConfigurationService, 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,
+ @inject(IConfigurationService) private configService: IConfigurationService
+ ) { }
+
+ /**
+ * 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<boolean> {
+ // 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)) {
+ 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<boolean> {
+ 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<boolean | undefined> {
+ 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): 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 get isFeatureEnabled(): boolean {
+ return !this.configService.getSettings().jediEnabled;
+ }
+}

src/client/linters/linterCommands.ts

@@ -28,7 +28,7 @@ export class LinterCommands implements vscode.Disposable {
  public async setLinterAsync(): Promise<void> {
  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(true, this.settingsUri);
 
  let current: string;
  switch (activeLinters.length) {
@@ -64,7 +64,7 @@ export class LinterCommands implements vscode.Disposable {
 
  public async enableLintingAsync(): Promise<void> {
  const options = ['on', 'off'];
- const current = this.linterManager.isLintingEnabled(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,

src/client/linters/linterManager.ts

@@ -1,9 +1,15 @@
 // 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, OutputChannel, TextDocument, Uri
+} from 'vscode';
+import {
+ IConfigurationService, ILogger, Product
+} from '../common/types';
 import { IServiceContainer } from '../ioc/types';
 import { Bandit } from './bandit';
 import { Flake8 } from './flake8';
@@ -14,7 +20,9 @@ import { Prospector } from './prospector';
 import { PyDocStyle } from './pydocstyle';
 import { PyLama } from './pylama';
 import { Pylint } from './pylint';
-import { ILinter, ILinterInfo, ILinterManager, ILintMessage } from './types';
+import {
+ IAvailableLinterActivator, ILinter, ILinterInfo, ILinterManager, ILintMessage
+} from './types';
 
 class DisabledLinter implements ILinter {
  constructor(private configService: IConfigurationService) { }
@@ -31,8 +39,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>(IConfigurationService);
  this.linters = [
  new LinterInfo(Product.bandit, 'bandit', this.configService),
@@ -58,40 +67,49 @@ export class LinterManager implements ILinterManager {
  throw new Error('Invalid linter');
  }
 
- public isLintingEnabled(resource?: Uri): boolean {
+ public async isLintingEnabled(silent: boolean, resource?: Uri): Promise<boolean> {
  const settings = this.configService.getSettings(resource);
- return (settings.linting[this.lintingEnabledSettingName] as boolean) && this.getActiveLinters(resource).length > 0;
+ const activeLintersPresent = await this.getActiveLinters(silent, resource);
+ return (settings.linting[this.lintingEnabledSettingName] as boolean) && activeLintersPresent.length > 0;
  }
 
  public async enableLintingAsync(enable: boolean, resource?: Uri): Promise<void> {
  await this.configService.updateSetting(`linting.${this.lintingEnabledSettingName}`, enable, resource);
-
- // If nothing is enabled, fix it up to PyLint (default).
- if (enable && this.getActiveLinters(resource).length === 0) {
- await this.setActiveLintersAsync([Product.pylint], resource);
- }
  }
 
- public getActiveLinters(resource?: Uri): ILinterInfo[] {
+ public async getActiveLinters(silent: boolean, resource?: Uri): Promise<ILinterInfo[]> {
+ if (!silent) {
+ await this.enableUnconfiguredLinters(resource);
+ }
  return this.linters.filter(x => x.isEnabled(resource));
  }
 
  public async setActiveLintersAsync(products: Product[], resource?: Uri): Promise<void> {
- const active = this.getActiveLinters(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);
  }
  }
 
- 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<ILinter> {
+ if (!await this.isLintingEnabled(true, resource)) {
  return new DisabledLinter(this.configService);
  }
  const error = 'Linter manager: Unknown linter';
@@ -118,4 +136,24 @@ 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;
+ }
+ 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>(IAvailableLinterActivator);
+ return activator.promptIfLinterAvailable(pylintInfo, resource);
+ }
+ return false;
+ }
 }

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<ILintMessage[]>[] = this.linterManager.getActiveLinters(document.uri)
- .map(info => {
+ const activeLinters = await this.linterManager.getActiveLinters(false, document.uri);
+ const promises: Promise<ILintMessage[]>[] = 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<boolean> {
- if (!this.linterManager.isLintingEnabled(document.uri)) {
+ if (!await this.linterManager.isLintingEnabled(false, document.uri)) {
  this.diagnosticCollection.set(document.uri, []);
  return false;
  }

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>(ILintingEngine, LintingEngine);
  serviceManager.addSingleton<ILinterManager>(ILinterManager, LinterManager);
+ serviceManager.add<IAvailableLinterActivator>(IAvailableLinterActivator, AvailableLinterActivator);
 }

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<void>;
+ enableAsync(enabled: boolean, resource?: vscode.Uri): Promise<void>;
  isEnabled(resource?: vscode.Uri): boolean;
  pathName(resource?: vscode.Uri): string;
  linterArgs(resource?: vscode.Uri): string[];
@@ -31,15 +31,20 @@ export interface ILinter {
  lint(document: vscode.TextDocument, cancellation: vscode.CancellationToken): Promise<ILintMessage[]>;
 }
 
+export const IAvailableLinterActivator = Symbol('IAvailableLinterActivator');
+export interface IAvailableLinterActivator {
+ promptIfLinterAvailable(linter: ILinterInfo, resource?: vscode.Uri): Promise<boolean>;
+}
+
 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(silent: boolean, resource?: vscode.Uri): Promise<ILinterInfo[]>;
+ isLintingEnabled(silent: boolean, resource?: vscode.Uri): Promise<boolean>;
  enableLintingAsync(enable: boolean, resource?: vscode.Uri): Promise<void>;
  setActiveLintersAsync(products: Product[], resource?: vscode.Uri): Promise<void>;
- createLinter(product: Product, outputChannel: vscode.OutputChannel, serviceContainer: IServiceContainer, resource?: vscode.Uri): ILinter;
+ createLinter(product: Product, outputChannel: vscode.OutputChannel, serviceContainer: IServiceContainer, resource?: vscode.Uri): Promise<ILinter>;
 }
 
 export interface ILintMessage {