Skip to content

Commit c46c583

Browse files
authored
Revert linter installation prompt removal (#16147)
* Re-add prompt to install a linter * News entry * Remove experiment group * Remove leftover experiments telemetry
1 parent c23fca4 commit c46c583

File tree

7 files changed

+309
-9
lines changed

7 files changed

+309
-9
lines changed

news/2 Fixes/16027.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Revert linter installation prompt removal.

src/client/common/installer/productInstaller.ts

Lines changed: 103 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,12 @@ import { CancellationToken, OutputChannel, Uri } from 'vscode';
77
import '../extensions';
88
import { IInterpreterService } from '../../interpreter/contracts';
99
import { IServiceContainer } from '../../ioc/types';
10+
import { LinterId } from '../../linters/types';
1011
import { EnvironmentType, PythonEnvironment } from '../../pythonEnvironments/info';
1112
import { sendTelemetryEvent } from '../../telemetry';
1213
import { EventName } from '../../telemetry/constants';
13-
import { IApplicationShell, IWorkspaceService } from '../application/types';
14-
import { STANDARD_OUTPUT_CHANNEL } from '../constants';
14+
import { IApplicationShell, ICommandManager, IWorkspaceService } from '../application/types';
15+
import { Commands, STANDARD_OUTPUT_CHANNEL } from '../constants';
1516
import { traceError, traceInfo } from '../logger';
1617
import { IPlatformService } from '../platform/types';
1718
import { IProcessServiceFactory, IPythonExecutionFactory } from '../process/types';
@@ -21,12 +22,13 @@ import {
2122
IInstaller,
2223
InstallerResponse,
2324
IOutputChannel,
25+
IPersistentStateFactory,
2426
ProductInstallStatus,
2527
ModuleNamePurpose,
2628
Product,
2729
ProductType,
2830
} from '../types';
29-
import { Installer } from '../utils/localize';
31+
import { Common, Installer, Linters } from '../utils/localize';
3032
import { isResource, noop } from '../utils/misc';
3133
import { translateProductToModule } from './moduleInstaller';
3234
import { ProductNames } from './productNames';
@@ -306,7 +308,102 @@ export class FormatterInstaller extends BaseInstaller {
306308
return InstallerResponse.Ignore;
307309
}
308310
}
309-
class TestFrameworkInstaller extends BaseInstaller {
311+
312+
export class LinterInstaller extends BaseInstaller {
313+
constructor(protected serviceContainer: IServiceContainer, protected outputChannel: OutputChannel) {
314+
super(serviceContainer, outputChannel);
315+
}
316+
317+
protected async promptToInstallImplementation(
318+
product: Product,
319+
resource?: Uri,
320+
cancel?: CancellationToken,
321+
): Promise<InstallerResponse> {
322+
sendTelemetryEvent(EventName.LINTER_INSTALL_PROMPT, undefined, {
323+
prompt: 'old',
324+
});
325+
326+
return this.oldPromptForInstallation(product, resource, cancel);
327+
}
328+
329+
/**
330+
* For installers that want to avoid prompting the user over and over, they can make use of a
331+
* persisted true/false value representing user responses to 'stop showing this prompt'. This method
332+
* gets the persisted value given the installer-defined key.
333+
*
334+
* @param key Key to use to get a persisted response value, each installer must define this for themselves.
335+
* @returns Boolean: The current state of the stored response key given.
336+
*/
337+
protected getStoredResponse(key: string): boolean {
338+
const factory = this.serviceContainer.get<IPersistentStateFactory>(IPersistentStateFactory);
339+
const state = factory.createGlobalPersistentState<boolean | undefined>(key, undefined);
340+
return state.value === true;
341+
}
342+
343+
private async oldPromptForInstallation(product: Product, resource?: Uri, cancel?: CancellationToken) {
344+
const productName = ProductNames.get(product)!;
345+
const install = Common.install();
346+
const doNotShowAgain = Common.doNotShowAgain();
347+
const disableLinterInstallPromptKey = `${productName}_DisableLinterInstallPrompt`;
348+
const selectLinter = Linters.selectLinter();
349+
350+
if (this.getStoredResponse(disableLinterInstallPromptKey) === true) {
351+
return InstallerResponse.Ignore;
352+
}
353+
354+
const options = [selectLinter, doNotShowAgain];
355+
356+
let message = `Linter ${productName} is not installed.`;
357+
if (this.isExecutableAModule(product, resource)) {
358+
options.splice(0, 0, install);
359+
} else {
360+
const executable = this.getExecutableNameFromSettings(product, resource);
361+
message = `Path to the ${productName} linter is invalid (${executable})`;
362+
}
363+
const response = await this.appShell.showErrorMessage(message, ...options);
364+
if (response === install) {
365+
sendTelemetryEvent(EventName.LINTER_NOT_INSTALLED_PROMPT, undefined, {
366+
tool: productName as LinterId,
367+
action: 'install',
368+
});
369+
return this.install(product, resource, cancel);
370+
}
371+
if (response === doNotShowAgain) {
372+
await this.setStoredResponse(disableLinterInstallPromptKey, true);
373+
sendTelemetryEvent(EventName.LINTER_NOT_INSTALLED_PROMPT, undefined, {
374+
tool: productName as LinterId,
375+
action: 'disablePrompt',
376+
});
377+
return InstallerResponse.Ignore;
378+
}
379+
380+
if (response === selectLinter) {
381+
sendTelemetryEvent(EventName.LINTER_NOT_INSTALLED_PROMPT, undefined, { action: 'select' });
382+
const commandManager = this.serviceContainer.get<ICommandManager>(ICommandManager);
383+
await commandManager.executeCommand(Commands.Set_Linter);
384+
}
385+
return InstallerResponse.Ignore;
386+
}
387+
388+
/**
389+
* For installers that want to avoid prompting the user over and over, they can make use of a
390+
* persisted true/false value representing user responses to 'stop showing this prompt'. This
391+
* method will set that persisted value given the installer-defined key.
392+
*
393+
* @param key Key to use to get a persisted response value, each installer must define this for themselves.
394+
* @param value Boolean value to store for the user - if they choose to not be prompted again for instance.
395+
* @returns Boolean: The current state of the stored response key given.
396+
*/
397+
private async setStoredResponse(key: string, value: boolean): Promise<void> {
398+
const factory = this.serviceContainer.get<IPersistentStateFactory>(IPersistentStateFactory);
399+
const state = factory.createGlobalPersistentState<boolean | undefined>(key, undefined);
400+
if (state && state.value !== value) {
401+
await state.updateValue(value);
402+
}
403+
}
404+
}
405+
406+
export class TestFrameworkInstaller extends BaseInstaller {
310407
protected async promptToInstallImplementation(
311408
product: Product,
312409
resource?: Uri,
@@ -490,6 +587,8 @@ export class ProductInstaller implements IInstaller {
490587
switch (productType) {
491588
case ProductType.Formatter:
492589
return new FormatterInstaller(this.serviceContainer, this.outputChannel);
590+
case ProductType.Linter:
591+
return new LinterInstaller(this.serviceContainer, this.outputChannel);
493592
case ProductType.WorkspaceSymbols:
494593
return new CTagsInstaller(this.serviceContainer, this.outputChannel);
495594
case ProductType.TestFramework:

src/client/telemetry/constants.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,8 @@ export enum EventName {
113113

114114
SELECT_LINTER = 'LINTING.SELECT',
115115

116+
LINTER_NOT_INSTALLED_PROMPT = 'LINTER_NOT_INSTALLED_PROMPT',
117+
LINTER_INSTALL_PROMPT = 'LINTER_INSTALL_PROMPT',
116118
CONFIGURE_AVAILABLE_LINTER_PROMPT = 'CONFIGURE_AVAILABLE_LINTER_PROMPT',
117119
HASHED_PACKAGE_NAME = 'HASHED_PACKAGE_NAME',
118120
HASHED_PACKAGE_PERF = 'HASHED_PACKAGE_PERF',

src/client/telemetry/index.ts

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -796,6 +796,39 @@ export interface IEventNamePropertyMapping {
796796
hashedName: string;
797797
};
798798
[EventName.HASHED_PACKAGE_PERF]: never | undefined;
799+
/**
800+
* Telemetry event sent with details of selection in prompt
801+
* `Prompt message` :- 'Linter ${productName} is not installed'
802+
*/
803+
[EventName.LINTER_NOT_INSTALLED_PROMPT]: {
804+
/**
805+
* Name of the linter
806+
*
807+
* @type {LinterId}
808+
*/
809+
tool?: LinterId;
810+
/**
811+
* `select` When 'Select linter' option is selected
812+
* `disablePrompt` When 'Do not show again' option is selected
813+
* `install` When 'Install' option is selected
814+
*
815+
* @type {('select' | 'disablePrompt' | 'install')}
816+
*/
817+
action: 'select' | 'disablePrompt' | 'install';
818+
};
819+
820+
/**
821+
* Telemetry event sent before showing the linter prompt to install
822+
* pylint or flake8.
823+
*/
824+
[EventName.LINTER_INSTALL_PROMPT]: {
825+
/**
826+
* Identify which prompt was shown.
827+
*
828+
* @type {('old' | 'noPrompt' | 'pylintFirst' | 'flake8first')}
829+
*/
830+
prompt: 'old' | 'noPrompt' | 'pylintFirst' | 'flake8first';
831+
};
799832

800833
/**
801834
* Telemetry event sent when installing modules

src/test/common/installer.test.ts

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -321,11 +321,9 @@ suite('Installer', () => {
321321
}
322322
getNamesAndValues<Product>(Product).forEach((prod) => {
323323
test(`Ensure isInstalled for Product: '${prod.name}' executes the right command`, async function () {
324-
const productType = new ProductService().getProductType(prod.value);
325-
if (productType === ProductType.DataScience || productType === ProductType.Linter) {
324+
if (new ProductService().getProductType(prod.value) === ProductType.DataScience) {
326325
return this.skip();
327326
}
328-
329327
ioc.serviceManager.addSingletonInstance<IModuleInstaller>(
330328
IModuleInstaller,
331329
new MockModuleInstaller('one', false),
@@ -361,7 +359,7 @@ suite('Installer', () => {
361359
getNamesAndValues<Product>(Product).forEach((prod) => {
362360
test(`Ensure install for Product: '${prod.name}' executes the right command in IModuleInstaller`, async function () {
363361
const productType = new ProductService().getProductType(prod.value);
364-
if (productType === ProductType.DataScience || productType === ProductType.Linter) {
362+
if (productType === ProductType.DataScience) {
365363
return this.skip();
366364
}
367365
ioc.serviceManager.addSingletonInstance<IModuleInstaller>(

0 commit comments

Comments
 (0)