Remove prompt to install a linter (#15646)

kimadeline · web-flow · commit f2145447a23c · 2021-03-18T12:29:38.000-07:00
* Remove prompt to install a linter
* Forgot to remove the experiment group lol
* Skip installing linter in single-workspace tests
* Forgot the other test lol
diff --git a/news/1 Enhancements/15465.md b/news/1 Enhancements/15465.md
@@ -0,0 +1 @@
+Remove prompt to install a linter when none are available.
diff --git a/src/client/common/experiments/groups.ts b/src/client/common/experiments/groups.ts
@@ -35,13 +35,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',
diff --git a/src/client/common/installer/productInstaller.ts b/src/client/common/installer/productInstaller.ts
@@ -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 {
@@ -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,
@@ -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:
diff --git a/src/client/telemetry/constants.ts b/src/client/telemetry/constants.ts
@@ -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',
diff --git a/src/client/telemetry/index.ts b/src/client/telemetry/index.ts
@@ -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
      */
diff --git a/src/test/common/installer.test.ts b/src/test/common/installer.test.ts
@@ -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),
@@ -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>(
diff --git a/src/test/common/installer/installer.unit.test.ts b/src/test/common/installer/installer.unit.test.ts
diff --git a/src/test/linters/lint.provider.test.ts b/src/test/linters/lint.provider.test.ts

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+Remove prompt to install a linter when none are available.`