Skip to content

Remove prompt to install a linter #15646

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
kimadeline merged 6 commits into from
Mar 18, 2021
Merged

Remove prompt to install a linter #15646

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1cedb1d
Remove prompt to install a linter
Mar 11, 2021
121e825
Forgot to remove the experiment group lol
Mar 11, 2021
1c778d9
Skip installing linter in single-workspace tests
Mar 12, 2021
c7e2f17
Forgot the other test lol
Mar 12, 2021
d37e19f
Merge branch 'main' into 15465-drop-pylint-notification
Mar 12, 2021
947a46a
Merge branch 'main' into 15465-drop-pylint-notification
Mar 17, 2021
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
1 change: 1 addition & 0 deletions news/1 Enhancements/15465.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Remove prompt to install a linter when none are available.
7 changes: 0 additions & 7 deletions src/client/common/experiments/groups.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,6 @@ export enum NativeTensorBoard {
experiment = 'pythonTensorboardExperiment',
}

// Experiment to show a prompt asking users to install or select linter
export enum LinterInstallationPromptVariants {
pylintFirst = 'pythonInstallPylintButtonFirst',
flake8First = 'pythonInstallFlake8ButtonFirst',
noPrompt = 'pythonNotDisplayLinterPrompt',
}

// Experiment to control which environment discovery mechanism can be used
export enum DiscoveryVariants {
discoverWithFileWatching = 'pythonDiscoveryModule',
Expand Down
252 changes: 3 additions & 249 deletions src/client/common/installer/productInstaller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,30 +5,24 @@ import { CancellationToken, OutputChannel, Uri } from 'vscode';
import '../../common/extensions';
import { IInterpreterService } from '../../interpreter/contracts';
import { IServiceContainer } from '../../ioc/types';
import { ILinterManager, LinterId } from '../../linters/types';
import { EnvironmentType, PythonEnvironment } from '../../pythonEnvironments/info';
import { sendTelemetryEvent } from '../../telemetry';
import { EventName } from '../../telemetry/constants';
import { IApplicationShell, ICommandManager, IWorkspaceService } from '../application/types';
import { Commands, STANDARD_OUTPUT_CHANNEL } from '../constants';
import { LinterInstallationPromptVariants } from '../experiments/groups';
import { IApplicationShell, IWorkspaceService } from '../application/types';
import { STANDARD_OUTPUT_CHANNEL } from '../constants';
import { traceError, traceInfo } from '../logger';
import { IPlatformService } from '../platform/types';
import { IProcessServiceFactory, IPythonExecutionFactory } from '../process/types';
import { ITerminalServiceFactory } from '../terminal/types';
import {
IConfigurationService,
IExperimentService,
IInstaller,
InstallerResponse,
IOutputChannel,
IPersistentStateFactory,
ProductInstallStatus,
ModuleNamePurpose,
Product,
ProductType,
} from '../types';
import { Common, Installer, Linters } from '../utils/localize';
import { Installer } from '../utils/localize';
import { isResource, noop } from '../utils/misc';
import { ProductNames } from './productNames';
import {
Expand Down Expand Up @@ -294,244 +288,6 @@ export class FormatterInstaller extends BaseInstaller {
return InstallerResponse.Ignore;
}
}

export class LinterInstaller extends BaseInstaller {
// This is a hack, really we should be handling this in a service that
// controls the prompts we show. The issue here was that if we show
// a prompt to install pylint and flake8, and user selects flake8
// we immediately show this prompt again saying install flake8, while the
// installation is on going.
private static promptSeen: boolean = false;
private readonly experimentsManager: IExperimentService;
private readonly linterManager: ILinterManager;

constructor(protected serviceContainer: IServiceContainer, protected outputChannel: OutputChannel) {
super(serviceContainer, outputChannel);
this.experimentsManager = serviceContainer.get<IExperimentService>(IExperimentService);
this.linterManager = serviceContainer.get<ILinterManager>(ILinterManager);
}

public static reset() {
// Read notes where this is defined.
LinterInstaller.promptSeen = false;
}

protected async promptToInstallImplementation(
product: Product,
resource?: Uri,
cancel?: CancellationToken,
): Promise<InstallerResponse> {
// This is a hack, really we should be handling this in a service that
// controls the prompts we show. The issue here was that if we show
// a prompt to install pylint and flake8, and user selects flake8
// we immediately show this prompt again saying install flake8, while the
// installation is on going.
if (LinterInstaller.promptSeen) {
return InstallerResponse.Ignore;
}

LinterInstaller.promptSeen = true;

// Conditions to use experiment prompt:
// 1. There should be no linter set in any scope
// 2. The default linter should be pylint

if (!this.isLinterSetInAnyScope() && product === Product.pylint) {
if (await this.experimentsManager.inExperiment(LinterInstallationPromptVariants.noPrompt)) {
// We won't show a prompt, so tell the extension to treat as though user
// ignored the prompt.
sendTelemetryEvent(EventName.LINTER_INSTALL_PROMPT, undefined, {
prompt: 'noPrompt',
});

const productName = ProductNames.get(product)!;
traceInfo(`Linter ${productName} is not installed.`);

return InstallerResponse.Ignore;
} else if (await this.experimentsManager.inExperiment(LinterInstallationPromptVariants.pylintFirst)) {
return this.newPromptForInstallation(true, resource, cancel);
} else if (await this.experimentsManager.inExperiment(LinterInstallationPromptVariants.flake8First)) {
return this.newPromptForInstallation(false, resource, cancel);
}
}

sendTelemetryEvent(EventName.LINTER_INSTALL_PROMPT, undefined, {
prompt: 'old',
});
return this.oldPromptForInstallation(product, resource, cancel);
}

/**
* For installers that want to avoid prompting the user over and over, they can make use of a
* persisted true/false value representing user responses to 'stop showing this prompt'. This method
* gets the persisted value given the installer-defined key.
*
* @param key Key to use to get a persisted response value, each installer must define this for themselves.
* @returns Boolean: The current state of the stored response key given.
*/
protected getStoredResponse(key: string): boolean {
const factory = this.serviceContainer.get<IPersistentStateFactory>(IPersistentStateFactory);
const state = factory.createGlobalPersistentState<boolean | undefined>(key, undefined);
return state.value === true;
}

private async newPromptForInstallation(pylintFirst: boolean, resource?: Uri, cancel?: CancellationToken) {
const productName = ProductNames.get(Product.pylint)!;

// User has already set to ignore this prompt
const disableLinterInstallPromptKey = `${productName}_DisableLinterInstallPrompt`;
if (this.getStoredResponse(disableLinterInstallPromptKey) === true) {
return InstallerResponse.Ignore;
}

// Check if the linter settings has Pylint or flake8 pointing to executables.
// If the settings point to executables then we can't install. Defer to old Prompt.
if (
!this.isExecutableAModule(Product.pylint, resource) ||
!this.isExecutableAModule(Product.flake8, resource)
) {
return this.oldPromptForInstallation(Product.pylint, resource, cancel);
}

const installPylint = Linters.installPylint();
const installFlake8 = Linters.installFlake8();
const doNotShowAgain = Common.doNotShowAgain();

const options = pylintFirst
? [installPylint, installFlake8, doNotShowAgain]
: [installFlake8, installPylint, doNotShowAgain];
const message = Linters.installMessage();
const prompt = pylintFirst ? 'pylintFirst' : 'flake8first';

sendTelemetryEvent(EventName.LINTER_INSTALL_PROMPT, undefined, {
prompt,
});

const response = await this.appShell.showInformationMessage(message, ...options);

if (response === installPylint) {
sendTelemetryEvent(EventName.LINTER_INSTALL_PROMPT_ACTION, undefined, {
prompt,
action: 'installPylint',
});
return this.install(Product.pylint, resource, cancel);
} else if (response === installFlake8) {
sendTelemetryEvent(EventName.LINTER_INSTALL_PROMPT_ACTION, undefined, {
prompt,
action: 'installFlake8',
});
await this.linterManager.setActiveLintersAsync([Product.flake8], resource);
return this.install(Product.flake8, resource, cancel);
} else if (response === doNotShowAgain) {
sendTelemetryEvent(EventName.LINTER_INSTALL_PROMPT_ACTION, undefined, {
prompt,
action: 'disablePrompt',
});
await this.setStoredResponse(disableLinterInstallPromptKey, true);
return InstallerResponse.Ignore;
}

sendTelemetryEvent(EventName.LINTER_INSTALL_PROMPT_ACTION, undefined, {
prompt,
action: 'close',
});
return InstallerResponse.Ignore;
}

private async oldPromptForInstallation(product: Product, resource?: Uri, cancel?: CancellationToken) {
const isPylint = product === Product.pylint;

const productName = ProductNames.get(product)!;
const install = Common.install();
const doNotShowAgain = Common.doNotShowAgain();
const disableLinterInstallPromptKey = `${productName}_DisableLinterInstallPrompt`;
const selectLinter = Linters.selectLinter();

if (isPylint && this.getStoredResponse(disableLinterInstallPromptKey) === true) {
return InstallerResponse.Ignore;
}

const options = isPylint ? [selectLinter, doNotShowAgain] : [selectLinter];

let message = `Linter ${productName} is not installed.`;
if (this.isExecutableAModule(product, resource)) {
options.splice(0, 0, install);
} else {
const executable = this.getExecutableNameFromSettings(product, resource);
message = `Path to the ${productName} linter is invalid (${executable})`;
}
const response = await this.appShell.showErrorMessage(message, ...options);
if (response === install) {
sendTelemetryEvent(EventName.LINTER_NOT_INSTALLED_PROMPT, undefined, {
tool: productName as LinterId,
action: 'install',
});
return this.install(product, resource, cancel);
} else if (response === doNotShowAgain) {
await this.setStoredResponse(disableLinterInstallPromptKey, true);
sendTelemetryEvent(EventName.LINTER_NOT_INSTALLED_PROMPT, undefined, {
tool: productName as LinterId,
action: 'disablePrompt',
});
return InstallerResponse.Ignore;
}

if (response === selectLinter) {
sendTelemetryEvent(EventName.LINTER_NOT_INSTALLED_PROMPT, undefined, { action: 'select' });
const commandManager = this.serviceContainer.get<ICommandManager>(ICommandManager);
await commandManager.executeCommand(Commands.Set_Linter);
}
return InstallerResponse.Ignore;
}

private isLinterSetInAnyScope() {
const config = this.workspaceService.getConfiguration('python');
if (config) {
const keys = [
'linting.pylintEnabled',
'linting.flake8Enabled',
'linting.banditEnabled',
'linting.mypyEnabled',
'linting.pycodestyleEnabled',
'linting.prospectorEnabled',
'linting.pydocstyleEnabled',
'linting.pylamaEnabled',
];

const values = keys.map((key) => {
const value = config.inspect<boolean>(key);
if (value) {
if (value.globalValue || value.workspaceValue || value.workspaceFolderValue) {
return 'linter set';
}
}
return 'no info';
});

return values.includes('linter set');
}

return false;
}

/**
* For installers that want to avoid prompting the user over and over, they can make use of a
* persisted true/false value representing user responses to 'stop showing this prompt'. This
* method will set that persisted value given the installer-defined key.
*
* @param key Key to use to get a persisted response value, each installer must define this for themselves.
* @param value Boolean value to store for the user - if they choose to not be prompted again for instance.
* @returns Boolean: The current state of the stored response key given.
*/
private async setStoredResponse(key: string, value: boolean): Promise<void> {
const factory = this.serviceContainer.get<IPersistentStateFactory>(IPersistentStateFactory);
const state = factory.createGlobalPersistentState<boolean | undefined>(key, undefined);
if (state && state.value !== value) {
await state.updateValue(value);
}
}
}

export class TestFrameworkInstaller extends BaseInstaller {
protected async promptToInstallImplementation(
product: Product,
Expand Down Expand Up @@ -695,8 +451,6 @@ export class ProductInstaller implements IInstaller {
switch (productType) {
case ProductType.Formatter:
return new FormatterInstaller(this.serviceContainer, this.outputChannel);
case ProductType.Linter:
return new LinterInstaller(this.serviceContainer, this.outputChannel);
case ProductType.WorkspaceSymbols:
return new CTagsInstaller(this.serviceContainer, this.outputChannel);
case ProductType.TestFramework:
Expand Down
3 changes: 0 additions & 3 deletions src/client/telemetry/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,9 +113,6 @@ export enum EventName {

SELECT_LINTER = 'LINTING.SELECT',

LINTER_NOT_INSTALLED_PROMPT = 'LINTER_NOT_INSTALLED_PROMPT',
LINTER_INSTALL_PROMPT = 'LINTER_INSTALL_PROMPT',
LINTER_INSTALL_PROMPT_ACTION = 'LINTER_INSTALL_PROMPT_ACTION',
CONFIGURE_AVAILABLE_LINTER_PROMPT = 'CONFIGURE_AVAILABLE_LINTER_PROMPT',
HASHED_PACKAGE_NAME = 'HASHED_PACKAGE_NAME',
HASHED_PACKAGE_PERF = 'HASHED_PACKAGE_PERF',
Expand Down
52 changes: 0 additions & 52 deletions src/client/telemetry/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -796,59 +796,7 @@ export interface IEventNamePropertyMapping {
hashedName: string;
};
[EventName.HASHED_PACKAGE_PERF]: never | undefined;
/**
* Telemetry event sent with details of selection in prompt
* `Prompt message` :- 'Linter ${productName} is not installed'
*/
[EventName.LINTER_NOT_INSTALLED_PROMPT]: {
/**
* Name of the linter
*
* @type {LinterId}
*/
tool?: LinterId;
/**
* `select` When 'Select linter' option is selected
* `disablePrompt` When 'Do not show again' option is selected
* `install` When 'Install' option is selected
*
* @type {('select' | 'disablePrompt' | 'install')}
*/
action: 'select' | 'disablePrompt' | 'install';
};

/**
* Telemetry event sent before showing the linter prompt to install
* pylint or flake8.
*/
[EventName.LINTER_INSTALL_PROMPT]: {
/**
* Identify which prompt was shown.
*
* @type {('old' | 'noPrompt' | 'pylintFirst' | 'flake8first')}
*/
prompt: 'old' | 'noPrompt' | 'pylintFirst' | 'flake8first';
};

/**
* Telemetry event sent after user had selected one of the options
* provided by the linter prompt.
*/
[EventName.LINTER_INSTALL_PROMPT_ACTION]: {
/**
* Identify which prompt was shown.
*
* @type {('pylintFirst' | 'flake8first')}
*/
prompt: 'pylintFirst' | 'flake8first';

/**
* Which of the following actions did user select
*
* @type {('pylint' | 'flake8' | 'disablePrompt' | 'close')}
*/
action: 'installPylint' | 'installFlake8' | 'disablePrompt' | 'close';
};
/**
* Telemetry event sent when installing modules
*/
Expand Down
6 changes: 4 additions & 2 deletions src/test/common/installer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,9 +328,11 @@ suite('Installer', () => {
}
getNamesAndValues<Product>(Product).forEach((prod) => {
test(`Ensure isInstalled for Product: '${prod.name}' executes the right command`, async function () {
if (new ProductService().getProductType(prod.value) === ProductType.DataScience) {
const productType = new ProductService().getProductType(prod.value);
if (productType === ProductType.DataScience || productType === ProductType.Linter) {
return this.skip();
}

ioc.serviceManager.addSingletonInstance<IModuleInstaller>(
IModuleInstaller,
new MockModuleInstaller('one', false),
Expand Down Expand Up @@ -365,7 +367,7 @@ suite('Installer', () => {
getNamesAndValues<Product>(Product).forEach((prod) => {
test(`Ensure install for Product: '${prod.name}' executes the right command in IModuleInstaller`, async function () {
const productType = new ProductService().getProductType(prod.value);
if (productType === ProductType.DataScience) {
if (productType === ProductType.DataScience || productType === ProductType.Linter) {
return this.skip();
}
ioc.serviceManager.addSingletonInstance<IModuleInstaller>(
Expand Down
Loading