diff --git a/news/2 Fixes/16027.md b/news/2 Fixes/16027.md new file mode 100644 index 000000000000..75061b3fe0ca --- /dev/null +++ b/news/2 Fixes/16027.md @@ -0,0 +1 @@ +Revert linter installation prompt removal. diff --git a/src/client/common/installer/productInstaller.ts b/src/client/common/installer/productInstaller.ts index fd65c2d725af..b130cf1c0aec 100644 --- a/src/client/common/installer/productInstaller.ts +++ b/src/client/common/installer/productInstaller.ts @@ -7,11 +7,12 @@ import { CancellationToken, OutputChannel, Uri } from 'vscode'; import '../extensions'; import { IInterpreterService } from '../../interpreter/contracts'; import { IServiceContainer } from '../../ioc/types'; +import { LinterId } from '../../linters/types'; import { EnvironmentType, PythonEnvironment } from '../../pythonEnvironments/info'; import { sendTelemetryEvent } from '../../telemetry'; import { EventName } from '../../telemetry/constants'; -import { IApplicationShell, IWorkspaceService } from '../application/types'; -import { STANDARD_OUTPUT_CHANNEL } from '../constants'; +import { IApplicationShell, ICommandManager, IWorkspaceService } from '../application/types'; +import { Commands, STANDARD_OUTPUT_CHANNEL } from '../constants'; import { traceError, traceInfo } from '../logger'; import { IPlatformService } from '../platform/types'; import { IProcessServiceFactory, IPythonExecutionFactory } from '../process/types'; @@ -21,12 +22,13 @@ import { IInstaller, InstallerResponse, IOutputChannel, + IPersistentStateFactory, ProductInstallStatus, ModuleNamePurpose, Product, ProductType, } from '../types'; -import { Installer } from '../utils/localize'; +import { Common, Installer, Linters } from '../utils/localize'; import { isResource, noop } from '../utils/misc'; import { translateProductToModule } from './moduleInstaller'; import { ProductNames } from './productNames'; @@ -306,7 +308,102 @@ export class FormatterInstaller extends BaseInstaller { return InstallerResponse.Ignore; } } -class TestFrameworkInstaller extends BaseInstaller { + +export class LinterInstaller extends BaseInstaller { + constructor(protected serviceContainer: IServiceContainer, protected outputChannel: OutputChannel) { + super(serviceContainer, outputChannel); + } + + protected async promptToInstallImplementation( + product: Product, + resource?: Uri, + cancel?: CancellationToken, + ): Promise { + 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); + const state = factory.createGlobalPersistentState(key, undefined); + return state.value === true; + } + + private async oldPromptForInstallation(product: Product, resource?: Uri, cancel?: CancellationToken) { + const productName = ProductNames.get(product)!; + const install = Common.install(); + const doNotShowAgain = Common.doNotShowAgain(); + const disableLinterInstallPromptKey = `${productName}_DisableLinterInstallPrompt`; + const selectLinter = Linters.selectLinter(); + + if (this.getStoredResponse(disableLinterInstallPromptKey) === true) { + return InstallerResponse.Ignore; + } + + const options = [selectLinter, doNotShowAgain]; + + 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); + } + 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); + await commandManager.executeCommand(Commands.Set_Linter); + } + return InstallerResponse.Ignore; + } + + /** + * 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 { + const factory = this.serviceContainer.get(IPersistentStateFactory); + const state = factory.createGlobalPersistentState(key, undefined); + if (state && state.value !== value) { + await state.updateValue(value); + } + } +} + +export class TestFrameworkInstaller extends BaseInstaller { protected async promptToInstallImplementation( product: Product, resource?: Uri, @@ -490,6 +587,8 @@ 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: diff --git a/src/client/telemetry/constants.ts b/src/client/telemetry/constants.ts index c30cb777c5de..3d3a65881d28 100644 --- a/src/client/telemetry/constants.ts +++ b/src/client/telemetry/constants.ts @@ -113,6 +113,8 @@ export enum EventName { SELECT_LINTER = 'LINTING.SELECT', + LINTER_NOT_INSTALLED_PROMPT = 'LINTER_NOT_INSTALLED_PROMPT', + LINTER_INSTALL_PROMPT = 'LINTER_INSTALL_PROMPT', CONFIGURE_AVAILABLE_LINTER_PROMPT = 'CONFIGURE_AVAILABLE_LINTER_PROMPT', HASHED_PACKAGE_NAME = 'HASHED_PACKAGE_NAME', HASHED_PACKAGE_PERF = 'HASHED_PACKAGE_PERF', diff --git a/src/client/telemetry/index.ts b/src/client/telemetry/index.ts index 46989cf6868a..aa3a25d7dc91 100644 --- a/src/client/telemetry/index.ts +++ b/src/client/telemetry/index.ts @@ -796,6 +796,39 @@ 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 when installing modules diff --git a/src/test/common/installer.test.ts b/src/test/common/installer.test.ts index 6ccff9994293..2f5626444778 100644 --- a/src/test/common/installer.test.ts +++ b/src/test/common/installer.test.ts @@ -321,11 +321,9 @@ suite('Installer', () => { } getNamesAndValues(Product).forEach((prod) => { test(`Ensure isInstalled for Product: '${prod.name}' executes the right command`, async function () { - const productType = new ProductService().getProductType(prod.value); - if (productType === ProductType.DataScience || productType === ProductType.Linter) { + if (new ProductService().getProductType(prod.value) === ProductType.DataScience) { return this.skip(); } - ioc.serviceManager.addSingletonInstance( IModuleInstaller, new MockModuleInstaller('one', false), @@ -361,7 +359,7 @@ suite('Installer', () => { getNamesAndValues(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 || productType === ProductType.Linter) { + if (productType === ProductType.DataScience) { return this.skip(); } ioc.serviceManager.addSingletonInstance( diff --git a/src/test/common/installer/installer.unit.test.ts b/src/test/common/installer/installer.unit.test.ts index 0d866bd17df8..849a1695125a 100644 --- a/src/test/common/installer/installer.unit.test.ts +++ b/src/test/common/installer/installer.unit.test.ts @@ -1,10 +1,12 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +/* eslint-disable max-classes-per-file */ + import { assert, expect, use } from 'chai'; import * as chaiAsPromised from 'chai-as-promised'; import * as sinon from 'sinon'; -import { instance, mock, verify, when } from 'ts-mockito'; +import { anything, instance, mock, verify, when } from 'ts-mockito'; import * as TypeMoq from 'typemoq'; import { Disposable, OutputChannel, Uri, WorkspaceFolder } from 'vscode'; import { ApplicationShell } from '../../../client/common/application/applicationShell'; @@ -12,13 +14,18 @@ import { CommandManager } from '../../../client/common/application/commandManage import { IApplicationShell, ICommandManager, IWorkspaceService } from '../../../client/common/application/types'; import { WorkspaceService } from '../../../client/common/application/workspace'; import { ConfigurationService } from '../../../client/common/configuration/service'; +import { Commands } from '../../../client/common/constants'; +import { ExperimentService } from '../../../client/common/experiments/service'; import '../../../client/common/extensions'; import { CTagsInstallationScript, CTagsInstaller, FormatterInstaller, + LinterInstaller, ProductInstaller, } from '../../../client/common/installer/productInstaller'; +import { ProductNames } from '../../../client/common/installer/productNames'; +import { LinterProductPathService } from '../../../client/common/installer/productPath'; import { ProductService } from '../../../client/common/installer/productService'; import { IInstallationChannelManager, @@ -38,6 +45,7 @@ import { ITerminalService, ITerminalServiceFactory } from '../../../client/commo import { IConfigurationService, IDisposableRegistry, + IExperimentService, InstallerResponse, IOutputChannel, IPersistentState, @@ -47,11 +55,15 @@ import { } from '../../../client/common/types'; import { createDeferred, Deferred } from '../../../client/common/utils/async'; import { getNamesAndValues } from '../../../client/common/utils/enum'; +import { Common, Linters } from '../../../client/common/utils/localize'; import { IInterpreterService } from '../../../client/interpreter/contracts'; import { ServiceContainer } from '../../../client/ioc/container'; import { IServiceContainer } from '../../../client/ioc/types'; +import { LinterManager } from '../../../client/linters/linterManager'; +import { ILinterManager } from '../../../client/linters/types'; import { PythonEnvironment } from '../../../client/pythonEnvironments/info'; import { sleep } from '../../common'; +import { MockWorkspaceConfiguration } from '../../startPage/mockWorkspaceConfig'; use(chaiAsPromised); @@ -921,3 +933,154 @@ suite('Module Installer only', () => { }); }); }); + +[undefined, Uri.file('resource')].forEach((resource) => { + suite(`Test LinterInstaller with resource: ${resource}`, () => { + class LinterInstallerTest extends LinterInstaller { + public isModuleExecutable = true; + + public async promptToInstallImplementation(product: Product, uri?: Uri): Promise { + return super.promptToInstallImplementation(product, uri); + } + + // eslint-disable-next-line class-methods-use-this + protected getStoredResponse(_key: string) { + return false; + } + + protected isExecutableAModule(_product: Product, _resource?: Uri) { + return this.isModuleExecutable; + } + } + + let installer: LinterInstallerTest; + let appShell: IApplicationShell; + let configService: IConfigurationService; + let workspaceService: IWorkspaceService; + let productService: IProductService; + let cmdManager: ICommandManager; + let experimentsService: IExperimentService; + let linterManager: ILinterManager; + let serviceContainer: IServiceContainer; + let productPathService: IProductPathService; + let outputChannel: TypeMoq.IMock; + setup(() => { + serviceContainer = mock(ServiceContainer); + appShell = mock(ApplicationShell); + configService = mock(ConfigurationService); + workspaceService = mock(WorkspaceService); + productService = mock(ProductService); + cmdManager = mock(CommandManager); + experimentsService = mock(ExperimentService); + linterManager = mock(LinterManager); + productPathService = mock(LinterProductPathService); + outputChannel = TypeMoq.Mock.ofType(); + + when(serviceContainer.get(IApplicationShell)).thenReturn(instance(appShell)); + when(serviceContainer.get(IConfigurationService)).thenReturn( + instance(configService), + ); + when(serviceContainer.get(IWorkspaceService)).thenReturn(instance(workspaceService)); + when(serviceContainer.get(IProductService)).thenReturn(instance(productService)); + when(serviceContainer.get(ICommandManager)).thenReturn(instance(cmdManager)); + + const exp = instance(experimentsService); + when(serviceContainer.get(IExperimentService)).thenReturn(exp); + when(experimentsService.inExperiment(anything())).thenResolve(false); + + when(serviceContainer.get(ILinterManager)).thenReturn(instance(linterManager)); + when(serviceContainer.get(IProductPathService, ProductType.Linter)).thenReturn( + instance(productPathService), + ); + + installer = new LinterInstallerTest(instance(serviceContainer), outputChannel.object); + }); + + teardown(() => { + sinon.restore(); + }); + + test('Ensure 3 options for pylint', async () => { + const product = Product.pylint; + const options = ['Select Linter', 'Do not show again']; + const productName = ProductNames.get(product)!; + + await installer.promptToInstallImplementation(product, resource); + + verify( + appShell.showErrorMessage(`Linter ${productName} is not installed.`, 'Install', options[0], options[1]), + ).once(); + }); + test('Ensure select linter command is invoked', async () => { + const product = Product.pylint; + const options = ['Select Linter', 'Do not show again']; + const productName = ProductNames.get(product)!; + when( + appShell.showErrorMessage(`Linter ${productName} is not installed.`, 'Install', options[0], options[1]), + ).thenResolve(('Select Linter' as unknown) as void); + when(cmdManager.executeCommand(Commands.Set_Linter)).thenResolve(undefined); + + const response = await installer.promptToInstallImplementation(product, resource); + + verify( + appShell.showErrorMessage(`Linter ${productName} is not installed.`, 'Install', options[0], options[1]), + ).once(); + verify(cmdManager.executeCommand(Commands.Set_Linter)).once(); + expect(response).to.be.equal(InstallerResponse.Ignore); + }); + test('If install button is selected, install linter and return response', async () => { + const product = Product.pylint; + const options = ['Select Linter', 'Do not show again']; + const productName = ProductNames.get(product)!; + when( + appShell.showErrorMessage(`Linter ${productName} is not installed.`, 'Install', options[0], options[1]), + ).thenResolve(('Install' as unknown) as void); + when(cmdManager.executeCommand(Commands.Set_Linter)).thenResolve(undefined); + const install = sinon.stub(LinterInstaller.prototype, 'install'); + install.resolves(InstallerResponse.Installed); + + const response = await installer.promptToInstallImplementation(product, resource); + + expect(response).to.be.equal(InstallerResponse.Installed); + assert.ok(install.calledOnceWith(product, resource, undefined)); + }); + + test('Do not show prompt if linter path is set', async () => { + when(workspaceService.getConfiguration('python')).thenReturn( + new MockWorkspaceConfiguration({ + 'linting.pylintPath': { + globalValue: 'path/to/something', + }, + }), + ); + when(productService.getProductType(Product.pylint)).thenReturn(ProductType.Linter); + when(productPathService.getExecutableNameFromSettings(Product.pylint, resource)).thenReturn( + 'path/to/something', + ); + installer.isModuleExecutable = false; + + const product = Product.pylint; + const options = ['Select Linter', 'Do not show again']; + const productName = ProductNames.get(product)!; + await installer.promptToInstallImplementation(product, resource); + verify( + appShell.showInformationMessage( + Linters.installMessage(), + Linters.installPylint(), + Linters.installFlake8(), + Common.doNotShowAgain(), + ), + ).never(); + verify( + appShell.showErrorMessage(`Linter ${productName} is not installed.`, 'Install', options[0], options[1]), + ).never(); + verify( + appShell.showErrorMessage( + `Path to the ${productName} linter is invalid (path/to/something)`, + options[0], + options[1], + ), + ).once(); + }); + }); +}); diff --git a/src/test/linters/lint.provider.test.ts b/src/test/linters/lint.provider.test.ts index 7600f59d6c9e..35c73300dfdd 100644 --- a/src/test/linters/lint.provider.test.ts +++ b/src/test/linters/lint.provider.test.ts @@ -18,6 +18,7 @@ import { IFileSystem } from '../../client/common/platform/types'; import { GLOBAL_MEMENTO, IConfigurationService, + IInstaller, ILintingSettings, IMemento, IPersistentStateFactory, @@ -53,6 +54,7 @@ suite('Linting - Provider', () => { let document: TypeMoq.IMock; let fs: TypeMoq.IMock; let appShell: TypeMoq.IMock; + let linterInstaller: TypeMoq.IMock; let workspaceService: TypeMoq.IMock; let workspaceConfig: TypeMoq.IMock; @@ -92,6 +94,7 @@ suite('Linting - Provider', () => { serviceManager.addSingletonInstance(IConfigurationService, configService.object); appShell = TypeMoq.Mock.ofType(); + linterInstaller = TypeMoq.Mock.ofType(); workspaceService = TypeMoq.Mock.ofType(); workspaceConfig = TypeMoq.Mock.ofType(); @@ -101,6 +104,7 @@ suite('Linting - Provider', () => { workspaceService.setup((w) => w.getConfiguration('python')).returns(() => workspaceConfig.object); serviceManager.addSingletonInstance(IApplicationShell, appShell.object); + serviceManager.addSingletonInstance(IInstaller, linterInstaller.object); serviceManager.addSingletonInstance(IWorkspaceService, workspaceService.object); serviceManager.add(IAvailableLinterActivator, AvailableLinterActivator); serviceManager.addSingleton(