From d585da00689f042c71bcbb395961bc24f9efec93 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Wed, 25 Jan 2023 16:36:04 -0800 Subject: [PATCH 1/4] Remove `isort` extension dependency --- gulpfile.js | 2 +- pythonFiles/sortImports.py | 14 --- .../common/process/internal/scripts/index.ts | 17 ---- src/client/common/utils/localize.ts | 4 + src/client/common/vscodeApis/extensionsApi.ts | 30 +++++++ src/client/linters/flake8.ts | 4 +- src/client/linters/prompts/common.ts | 12 +++ src/client/linters/pylint.ts | 4 +- .../codeActionProvider/isortPrompt.ts | 87 +++++++++++++++++++ .../providers/codeActionProvider/main.ts | 22 ++++- src/client/telemetry/index.ts | 8 +- .../codeActionProvider/main.unit.test.ts | 6 +- 12 files changed, 169 insertions(+), 41 deletions(-) delete mode 100644 pythonFiles/sortImports.py create mode 100644 src/client/common/vscodeApis/extensionsApi.ts create mode 100644 src/client/providers/codeActionProvider/isortPrompt.ts diff --git a/gulpfile.js b/gulpfile.js index cb901ad9f10c..2949490abd66 100644 --- a/gulpfile.js +++ b/gulpfile.js @@ -82,7 +82,7 @@ async function addExtensionPackDependencies() { // extension dependencies need not be installed during development const packageJsonContents = await fsExtra.readFile('package.json', 'utf-8'); const packageJson = JSON.parse(packageJsonContents); - packageJson.extensionPack = ['ms-toolsai.jupyter', 'ms-python.vscode-pylance', 'ms-python.isort'].concat( + packageJson.extensionPack = ['ms-toolsai.jupyter', 'ms-python.vscode-pylance'].concat( packageJson.extensionPack ? packageJson.extensionPack : [], ); await fsExtra.writeFile('package.json', JSON.stringify(packageJson, null, 4), 'utf-8'); diff --git a/pythonFiles/sortImports.py b/pythonFiles/sortImports.py deleted file mode 100644 index 070f7883fd66..000000000000 --- a/pythonFiles/sortImports.py +++ /dev/null @@ -1,14 +0,0 @@ -# Copyright (c) Microsoft Corporation. All rights reserved. -# Licensed under the MIT License. - -import io -import os -import os.path -import sys - -isort_path = os.path.join(os.path.dirname(__file__), "lib", "python") -sys.path.insert(0, isort_path) - -import isort.main - -isort.main.main() diff --git a/src/client/common/process/internal/scripts/index.ts b/src/client/common/process/internal/scripts/index.ts index 33eaa4686e8f..c52983d9910b 100644 --- a/src/client/common/process/internal/scripts/index.ts +++ b/src/client/common/process/internal/scripts/index.ts @@ -58,23 +58,6 @@ export function interpreterInfo(): [string[], (out: string) => InterpreterInfoJs return [args, parse]; } -// sortImports.py - -export function sortImports(filename: string, sortArgs?: string[]): [string[], (out: string) => string] { - const script = path.join(SCRIPTS_DIR, 'sortImports.py'); - const args = [script, filename, '--diff']; - if (sortArgs) { - args.push(...sortArgs); - } - - function parse(out: string) { - // It should just be a diff that the extension will use directly. - return out; - } - - return [args, parse]; -} - // normalizeSelection.py export function normalizeSelection(): [string[], (out: string) => string] { diff --git a/src/client/common/utils/localize.ts b/src/client/common/utils/localize.ts index 6c8dbcf06663..60cbfd295f88 100644 --- a/src/client/common/utils/localize.ts +++ b/src/client/common/utils/localize.ts @@ -466,6 +466,10 @@ export namespace ToolsExtensions { export const pylintPromptMessage = l10n.t( 'Use the Pylint extension to enable easier configuration and new features such as quick fixes.', ); + export const isortPromptMessage = l10n.t( + 'To use sort imports, please install the isort extension. It provides easier configuration and new features such as code actions.', + ); export const installPylintExtension = l10n.t('Install Pylint extension'); export const installFlake8Extension = l10n.t('Install Flake8 extension'); + export const installISortExtension = l10n.t('Install isort extension'); } diff --git a/src/client/common/vscodeApis/extensionsApi.ts b/src/client/common/vscodeApis/extensionsApi.ts new file mode 100644 index 000000000000..27e0657f0687 --- /dev/null +++ b/src/client/common/vscodeApis/extensionsApi.ts @@ -0,0 +1,30 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import * as path from 'path'; +import * as fs from 'fs-extra'; +import { Extension, extensions } from 'vscode'; +import { PVSC_EXTENSION_ID } from '../constants'; + +export function getExtension(extensionId: string): Extension | undefined { + return extensions.getExtension(extensionId); +} + +export function isExtensionEnabled(extensionId: string): boolean { + return extensions.getExtension(extensionId) !== undefined; +} + +export function isExtensionDisabled(extensionId: string): boolean { + // We need an enabled extension to find the extensions dir. + const pythonExt = getExtension(PVSC_EXTENSION_ID); + if (pythonExt) { + let found = false; + fs.readdirSync(path.dirname(pythonExt.extensionPath), { withFileTypes: false }).forEach((s) => { + if (s.toString().startsWith(extensionId)) { + found = true; + } + }); + return found; + } + return false; +} diff --git a/src/client/linters/flake8.ts b/src/client/linters/flake8.ts index bb55b3be8706..3d6ac6c7b34e 100644 --- a/src/client/linters/flake8.ts +++ b/src/client/linters/flake8.ts @@ -4,6 +4,8 @@ import { Product } from '../common/types'; import { IServiceContainer } from '../ioc/types'; import { traceLog } from '../logging'; import { BaseLinter } from './baseLinter'; +import { isExtensionEnabled } from './prompts/common'; +import { FLAKE8_EXTENSION } from './prompts/flake8Prompt'; import { IToolsExtensionPrompt } from './prompts/types'; import { ILintMessage } from './types'; @@ -15,7 +17,7 @@ export class Flake8 extends BaseLinter { } protected async runLinter(document: TextDocument, cancellation: CancellationToken): Promise { - if (await this.prompt.showPrompt()) { + if ((await this.prompt.showPrompt()) && isExtensionEnabled(this.serviceContainer, FLAKE8_EXTENSION)) { traceLog('LINTING: Skipping linting from Python extension, since Flake8 extension is installed.'); return []; } diff --git a/src/client/linters/prompts/common.ts b/src/client/linters/prompts/common.ts index bf459f895ead..04c9735a9bec 100644 --- a/src/client/linters/prompts/common.ts +++ b/src/client/linters/prompts/common.ts @@ -26,6 +26,9 @@ function isExtensionInstalledButDisabled(extensions: IExtensions, extensionId: s return false; } +/** + * Detects installations even if extension is disabled. + */ export function isExtensionInstalled(serviceContainer: IServiceContainer, extensionId: string): boolean { const extensions: IExtensions = serviceContainer.get(IExtensions); const extension = extensions.getExtension(extensionId); @@ -36,6 +39,15 @@ export function isExtensionInstalled(serviceContainer: IServiceContainer, extens return extension !== undefined; } +/** + * Detects if extension is installed and enabled. + */ +export function isExtensionEnabled(serviceContainer: IServiceContainer, extensionId: string): boolean { + const extensions: IExtensions = serviceContainer.get(IExtensions); + const extension = extensions.getExtension(extensionId); + return extension !== undefined; +} + export function doNotShowPromptState( serviceContainer: IServiceContainer, promptKey: string, diff --git a/src/client/linters/pylint.ts b/src/client/linters/pylint.ts index 250f363a6e67..3e78d7062d0e 100644 --- a/src/client/linters/pylint.ts +++ b/src/client/linters/pylint.ts @@ -7,6 +7,8 @@ import { Product } from '../common/types'; import { IServiceContainer } from '../ioc/types'; import { traceError, traceLog } from '../logging'; import { BaseLinter } from './baseLinter'; +import { isExtensionEnabled } from './prompts/common'; +import { PYLINT_EXTENSION } from './prompts/pylintPrompt'; import { IToolsExtensionPrompt } from './prompts/types'; import { ILintMessage } from './types'; @@ -26,7 +28,7 @@ export class Pylint extends BaseLinter { } protected async runLinter(document: TextDocument, cancellation: CancellationToken): Promise { - if (await this.prompt.showPrompt()) { + if ((await this.prompt.showPrompt()) && isExtensionEnabled(this.serviceContainer, PYLINT_EXTENSION)) { traceLog('LINTING: Skipping linting from Python extension, since Pylint extension is installed.'); return []; } diff --git a/src/client/providers/codeActionProvider/isortPrompt.ts b/src/client/providers/codeActionProvider/isortPrompt.ts new file mode 100644 index 000000000000..83d3ef8aeb28 --- /dev/null +++ b/src/client/providers/codeActionProvider/isortPrompt.ts @@ -0,0 +1,87 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import { IApplicationEnvironment } from '../../common/application/types'; +import { IPersistentState, IPersistentStateFactory } from '../../common/types'; +import { Common, ToolsExtensions } from '../../common/utils/localize'; +import { executeCommand } from '../../common/vscodeApis/commandApis'; +import { isExtensionDisabled, isExtensionEnabled } from '../../common/vscodeApis/extensionsApi'; +import { showInformationMessage } from '../../common/vscodeApis/windowApis'; +import { IServiceContainer } from '../../ioc/types'; +import { sendTelemetryEvent } from '../../telemetry'; +import { EventName } from '../../telemetry/constants'; + +export const ISORT_EXTENSION = 'ms-python.isort'; +const ISORT_PROMPT_DONOTSHOW_KEY = 'showISortExtensionPrompt'; + +function doNotShowPromptState(serviceContainer: IServiceContainer, promptKey: string): IPersistentState { + const persistFactory: IPersistentStateFactory = serviceContainer.get( + IPersistentStateFactory, + ); + return persistFactory.createWorkspacePersistentState(promptKey, false); +} + +export class ISortExtensionPrompt { + private shownThisSession = false; + + public constructor(private readonly serviceContainer: IServiceContainer) {} + + public async showPrompt(): Promise { + if (isExtensionEnabled(ISORT_EXTENSION) || isExtensionDisabled(ISORT_EXTENSION)) { + sendTelemetryEvent(EventName.TOOLS_EXTENSIONS_ALREADY_INSTALLED, undefined, { + extensionId: ISORT_EXTENSION, + }); + return true; + } + + const doNotShow = doNotShowPromptState(this.serviceContainer, ISORT_PROMPT_DONOTSHOW_KEY); + if (this.shownThisSession || doNotShow.value) { + return false; + } + + sendTelemetryEvent(EventName.TOOLS_EXTENSIONS_PROMPT_SHOWN, undefined, { extensionId: ISORT_EXTENSION }); + this.shownThisSession = true; + const response = await showInformationMessage( + ToolsExtensions.isortPromptMessage, + ToolsExtensions.installISortExtension, + Common.doNotShowAgain, + ); + + if (response === Common.doNotShowAgain) { + await doNotShow.updateValue(true); + sendTelemetryEvent(EventName.TOOLS_EXTENSIONS_PROMPT_DISMISSED, undefined, { + extensionId: ISORT_EXTENSION, + dismissType: 'doNotShow', + }); + return false; + } + + if (response === ToolsExtensions.installISortExtension) { + sendTelemetryEvent(EventName.TOOLS_EXTENSIONS_INSTALL_SELECTED, undefined, { + extensionId: ISORT_EXTENSION, + }); + const appEnv: IApplicationEnvironment = this.serviceContainer.get( + IApplicationEnvironment, + ); + await executeCommand('workbench.extensions.installExtension', ISORT_EXTENSION, { + installPreReleaseVersion: appEnv.extensionChannel === 'insiders', + }); + return true; + } + + sendTelemetryEvent(EventName.TOOLS_EXTENSIONS_PROMPT_DISMISSED, undefined, { + extensionId: ISORT_EXTENSION, + dismissType: 'close', + }); + + return false; + } +} + +let _prompt: ISortExtensionPrompt | undefined; +export function getOrCreateISortPrompt(serviceContainer: IServiceContainer): ISortExtensionPrompt { + if (!_prompt) { + _prompt = new ISortExtensionPrompt(serviceContainer); + } + return _prompt; +} diff --git a/src/client/providers/codeActionProvider/main.ts b/src/client/providers/codeActionProvider/main.ts index e8126ac2e95c..40afd4dbb2b2 100644 --- a/src/client/providers/codeActionProvider/main.ts +++ b/src/client/providers/codeActionProvider/main.ts @@ -7,13 +7,20 @@ import { IExtensionSingleActivationService } from '../../activation/types'; import { Commands } from '../../common/constants'; import { IDisposableRegistry } from '../../common/types'; import { executeCommand, registerCommand } from '../../common/vscodeApis/commandApis'; +import { isExtensionEnabled } from '../../common/vscodeApis/extensionsApi'; +import { IServiceContainer } from '../../ioc/types'; +import { traceLog } from '../../logging'; +import { getOrCreateISortPrompt, ISORT_EXTENSION } from './isortPrompt'; import { LaunchJsonCodeActionProvider } from './launchJsonCodeActionProvider'; @injectable() export class CodeActionProviderService implements IExtensionSingleActivationService { public readonly supportedWorkspaceTypes = { untrustedWorkspace: false, virtualWorkspace: false }; - constructor(@inject(IDisposableRegistry) private disposableRegistry: IDisposableRegistry) {} + constructor( + @inject(IDisposableRegistry) private disposableRegistry: IDisposableRegistry, + @inject(IServiceContainer) private serviceContainer: IServiceContainer, + ) {} public async activate(): Promise { // eslint-disable-next-line global-require @@ -29,7 +36,18 @@ export class CodeActionProviderService implements IExtensionSingleActivationServ }), ); this.disposableRegistry.push( - registerCommand(Commands.Sort_Imports, () => executeCommand('editor.action.organizeImports')), + registerCommand(Commands.Sort_Imports, async () => { + const prompt = getOrCreateISortPrompt(this.serviceContainer); + await prompt.showPrompt(); + if (!isExtensionEnabled(ISORT_EXTENSION)) { + traceLog( + 'Sort Imports: Please install and enable `ms-python.isort` extension to use this feature.', + ); + return; + } + + executeCommand('editor.action.organizeImports'); + }), ); } } diff --git a/src/client/telemetry/index.ts b/src/client/telemetry/index.ts index 50947d8456e9..5411026ecee0 100644 --- a/src/client/telemetry/index.ts +++ b/src/client/telemetry/index.ts @@ -2080,7 +2080,7 @@ export interface IEventNamePropertyMapping { } */ [EventName.TOOLS_EXTENSIONS_ALREADY_INSTALLED]: { - extensionId: 'ms-python.pylint' | 'ms-python.flake8'; + extensionId: 'ms-python.pylint' | 'ms-python.flake8' | 'ms-python.isort'; }; /** * Telemetry event sent when install linter or formatter extension prompt is shown. @@ -2091,7 +2091,7 @@ export interface IEventNamePropertyMapping { } */ [EventName.TOOLS_EXTENSIONS_PROMPT_SHOWN]: { - extensionId: 'ms-python.pylint' | 'ms-python.flake8'; + extensionId: 'ms-python.pylint' | 'ms-python.flake8' | 'ms-python.isort'; }; /** * Telemetry event sent when clicking to install linter or formatter extension from the suggestion prompt. @@ -2102,7 +2102,7 @@ export interface IEventNamePropertyMapping { } */ [EventName.TOOLS_EXTENSIONS_INSTALL_SELECTED]: { - extensionId: 'ms-python.pylint' | 'ms-python.flake8'; + extensionId: 'ms-python.pylint' | 'ms-python.flake8' | 'ms-python.isort'; }; /** * Telemetry event sent when dismissing prompt suggesting to install the linter or formatter extension. @@ -2114,7 +2114,7 @@ export interface IEventNamePropertyMapping { } */ [EventName.TOOLS_EXTENSIONS_PROMPT_DISMISSED]: { - extensionId: 'ms-python.pylint' | 'ms-python.flake8'; + extensionId: 'ms-python.pylint' | 'ms-python.flake8' | 'ms-python.isort'; dismissType: 'close' | 'doNotShow'; }; /* __GDPR__ diff --git a/src/test/providers/codeActionProvider/main.unit.test.ts b/src/test/providers/codeActionProvider/main.unit.test.ts index 55644d80ae54..501c3c7eca2b 100644 --- a/src/test/providers/codeActionProvider/main.unit.test.ts +++ b/src/test/providers/codeActionProvider/main.unit.test.ts @@ -8,6 +8,7 @@ import rewiremock from 'rewiremock'; import * as typemoq from 'typemoq'; import { CodeActionKind, CodeActionProvider, CodeActionProviderMetadata, DocumentSelector } from 'vscode'; import { IDisposableRegistry } from '../../../client/common/types'; +import { IServiceContainer } from '../../../client/ioc/types'; import { LaunchJsonCodeActionProvider } from '../../../client/providers/codeActionProvider/launchJsonCodeActionProvider'; import { CodeActionProviderService } from '../../../client/providers/codeActionProvider/main'; @@ -37,7 +38,10 @@ suite('Code Action Provider service', async () => { }; rewiremock.enable(); rewiremock('vscode').with(vscodeMock); - const quickFixService = new CodeActionProviderService(typemoq.Mock.ofType().object); + const quickFixService = new CodeActionProviderService( + typemoq.Mock.ofType().object, + typemoq.Mock.ofType().object, + ); await quickFixService.activate(); From 521fcceff94f6dae21bb6272fbea97b0f2247c79 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Mon, 6 Feb 2023 11:31:38 -0800 Subject: [PATCH 2/4] Tweak prompting. --- src/client/linters/flake8.ts | 8 ++++++-- src/client/linters/pylint.ts | 8 ++++++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/client/linters/flake8.ts b/src/client/linters/flake8.ts index 3d6ac6c7b34e..e79d09158741 100644 --- a/src/client/linters/flake8.ts +++ b/src/client/linters/flake8.ts @@ -17,8 +17,12 @@ export class Flake8 extends BaseLinter { } protected async runLinter(document: TextDocument, cancellation: CancellationToken): Promise { - if ((await this.prompt.showPrompt()) && isExtensionEnabled(this.serviceContainer, FLAKE8_EXTENSION)) { - traceLog('LINTING: Skipping linting from Python extension, since Flake8 extension is installed.'); + await this.prompt.showPrompt(); + + if (isExtensionEnabled(this.serviceContainer, FLAKE8_EXTENSION)) { + traceLog( + 'LINTING: Skipping linting from Python extension, since Flake8 extension is installed and enabled.', + ); return []; } diff --git a/src/client/linters/pylint.ts b/src/client/linters/pylint.ts index 3e78d7062d0e..0b635417f906 100644 --- a/src/client/linters/pylint.ts +++ b/src/client/linters/pylint.ts @@ -28,8 +28,12 @@ export class Pylint extends BaseLinter { } protected async runLinter(document: TextDocument, cancellation: CancellationToken): Promise { - if ((await this.prompt.showPrompt()) && isExtensionEnabled(this.serviceContainer, PYLINT_EXTENSION)) { - traceLog('LINTING: Skipping linting from Python extension, since Pylint extension is installed.'); + await this.prompt.showPrompt(); + + if (isExtensionEnabled(this.serviceContainer, PYLINT_EXTENSION)) { + traceLog( + 'LINTING: Skipping linting from Python extension, since Pylint extension is installed and enabled.', + ); return []; } From 4a36d533926ab8fa325bf2c28f7f1a8e1a89d7e2 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Mon, 6 Feb 2023 11:51:55 -0800 Subject: [PATCH 3/4] Improve prompt telemetry --- src/client/linters/prompts/common.ts | 16 +------- src/client/linters/prompts/flake8Prompt.ts | 6 ++- src/client/linters/prompts/pylintPrompt.ts | 6 ++- .../codeActionProvider/isortPrompt.ts | 4 +- src/client/telemetry/index.ts | 1 + src/test/linters/lint.functional.test.ts | 9 ++-- src/test/linters/lint.unit.test.ts | 9 ++-- .../linters/prompts/flake8Prompt.unit.test.ts | 41 ++++++++++++++----- .../linters/prompts/pylintPrompt.unit.test.ts | 39 +++++++++++------- 9 files changed, 82 insertions(+), 49 deletions(-) diff --git a/src/client/linters/prompts/common.ts b/src/client/linters/prompts/common.ts index 04c9735a9bec..ab88282db607 100644 --- a/src/client/linters/prompts/common.ts +++ b/src/client/linters/prompts/common.ts @@ -8,7 +8,8 @@ import { IExperimentService, IExtensions, IPersistentState, IPersistentStateFact import { IServiceContainer } from '../../ioc/types'; import { traceLog } from '../../logging'; -function isExtensionInstalledButDisabled(extensions: IExtensions, extensionId: string): boolean { +export function isExtensionDisabled(serviceContainer: IServiceContainer, extensionId: string): boolean { + const extensions: IExtensions = serviceContainer.get(IExtensions); // When debugging the python extension this `extensionPath` below will point to your repo. // If you are debugging this feature then set the `extensionPath` to right location after // the next line. @@ -26,19 +27,6 @@ function isExtensionInstalledButDisabled(extensions: IExtensions, extensionId: s return false; } -/** - * Detects installations even if extension is disabled. - */ -export function isExtensionInstalled(serviceContainer: IServiceContainer, extensionId: string): boolean { - const extensions: IExtensions = serviceContainer.get(IExtensions); - const extension = extensions.getExtension(extensionId); - if (!extension) { - // The extension you are looking for might be disabled. - return isExtensionInstalledButDisabled(extensions, extensionId); - } - return extension !== undefined; -} - /** * Detects if extension is installed and enabled. */ diff --git a/src/client/linters/prompts/flake8Prompt.ts b/src/client/linters/prompts/flake8Prompt.ts index c60767f2fa8d..fa1969df682a 100644 --- a/src/client/linters/prompts/flake8Prompt.ts +++ b/src/client/linters/prompts/flake8Prompt.ts @@ -8,7 +8,7 @@ import { showInformationMessage } from '../../common/vscodeApis/windowApis'; import { IServiceContainer } from '../../ioc/types'; import { sendTelemetryEvent } from '../../telemetry'; import { EventName } from '../../telemetry/constants'; -import { isExtensionInstalled, doNotShowPromptState, inToolsExtensionsExperiment } from './common'; +import { doNotShowPromptState, inToolsExtensionsExperiment, isExtensionDisabled, isExtensionEnabled } from './common'; import { IToolsExtensionPrompt } from './types'; export const FLAKE8_EXTENSION = 'ms-python.flake8'; @@ -20,9 +20,11 @@ export class Flake8ExtensionPrompt implements IToolsExtensionPrompt { public constructor(private readonly serviceContainer: IServiceContainer) {} public async showPrompt(): Promise { - if (isExtensionInstalled(this.serviceContainer, FLAKE8_EXTENSION)) { + const isEnabled = isExtensionEnabled(this.serviceContainer, FLAKE8_EXTENSION); + if (isEnabled || isExtensionDisabled(this.serviceContainer, FLAKE8_EXTENSION)) { sendTelemetryEvent(EventName.TOOLS_EXTENSIONS_ALREADY_INSTALLED, undefined, { extensionId: FLAKE8_EXTENSION, + isEnabled, }); return true; } diff --git a/src/client/linters/prompts/pylintPrompt.ts b/src/client/linters/prompts/pylintPrompt.ts index 7a0693740b32..37e583243078 100644 --- a/src/client/linters/prompts/pylintPrompt.ts +++ b/src/client/linters/prompts/pylintPrompt.ts @@ -8,7 +8,7 @@ import { showInformationMessage } from '../../common/vscodeApis/windowApis'; import { IServiceContainer } from '../../ioc/types'; import { sendTelemetryEvent } from '../../telemetry'; import { EventName } from '../../telemetry/constants'; -import { doNotShowPromptState, inToolsExtensionsExperiment, isExtensionInstalled } from './common'; +import { doNotShowPromptState, inToolsExtensionsExperiment, isExtensionDisabled, isExtensionEnabled } from './common'; import { IToolsExtensionPrompt } from './types'; export const PYLINT_EXTENSION = 'ms-python.pylint'; @@ -20,9 +20,11 @@ export class PylintExtensionPrompt implements IToolsExtensionPrompt { public constructor(private readonly serviceContainer: IServiceContainer) {} public async showPrompt(): Promise { - if (isExtensionInstalled(this.serviceContainer, PYLINT_EXTENSION)) { + const isEnabled = isExtensionEnabled(this.serviceContainer, PYLINT_EXTENSION); + if (isEnabled || isExtensionDisabled(this.serviceContainer, PYLINT_EXTENSION)) { sendTelemetryEvent(EventName.TOOLS_EXTENSIONS_ALREADY_INSTALLED, undefined, { extensionId: PYLINT_EXTENSION, + isEnabled, }); return true; } diff --git a/src/client/providers/codeActionProvider/isortPrompt.ts b/src/client/providers/codeActionProvider/isortPrompt.ts index 83d3ef8aeb28..ffef481b498d 100644 --- a/src/client/providers/codeActionProvider/isortPrompt.ts +++ b/src/client/providers/codeActionProvider/isortPrompt.ts @@ -27,9 +27,11 @@ export class ISortExtensionPrompt { public constructor(private readonly serviceContainer: IServiceContainer) {} public async showPrompt(): Promise { - if (isExtensionEnabled(ISORT_EXTENSION) || isExtensionDisabled(ISORT_EXTENSION)) { + const isEnabled = isExtensionEnabled(ISORT_EXTENSION); + if (isEnabled || isExtensionDisabled(ISORT_EXTENSION)) { sendTelemetryEvent(EventName.TOOLS_EXTENSIONS_ALREADY_INSTALLED, undefined, { extensionId: ISORT_EXTENSION, + isEnabled, }); return true; } diff --git a/src/client/telemetry/index.ts b/src/client/telemetry/index.ts index 5411026ecee0..41cf311c4d44 100644 --- a/src/client/telemetry/index.ts +++ b/src/client/telemetry/index.ts @@ -2081,6 +2081,7 @@ export interface IEventNamePropertyMapping { */ [EventName.TOOLS_EXTENSIONS_ALREADY_INSTALLED]: { extensionId: 'ms-python.pylint' | 'ms-python.flake8' | 'ms-python.isort'; + isEnabled: boolean; }; /** * Telemetry event sent when install linter or formatter extension prompt is shown. diff --git a/src/test/linters/lint.functional.test.ts b/src/test/linters/lint.functional.test.ts index 4c06a26067a5..a3dc70b7c21e 100644 --- a/src/test/linters/lint.functional.test.ts +++ b/src/test/linters/lint.functional.test.ts @@ -761,13 +761,16 @@ class TestFixture extends BaseTestFixture { } suite('Linting Functional Tests', () => { - let isExtensionInstalledStub: sinon.SinonStub; + let isExtensionEnabledStub: sinon.SinonStub; + let isExtensionDisabledStub: sinon.SinonStub; let doNotShowPromptStateStub: sinon.SinonStub; let persistentState: TypeMoq.IMock>; setup(() => { - isExtensionInstalledStub = sinon.stub(promptApis, 'isExtensionInstalled'); + isExtensionEnabledStub = sinon.stub(promptApis, 'isExtensionEnabled'); + isExtensionDisabledStub = sinon.stub(promptApis, 'isExtensionDisabled'); // For these tests we assume that linter extensions are not installed. - isExtensionInstalledStub.returns(false); + isExtensionEnabledStub.returns(false); + isExtensionDisabledStub.returns(false); persistentState = TypeMoq.Mock.ofType>(); persistentState.setup((p) => p.value).returns(() => true); diff --git a/src/test/linters/lint.unit.test.ts b/src/test/linters/lint.unit.test.ts index b20d19b2ceec..02bdd4c82c79 100644 --- a/src/test/linters/lint.unit.test.ts +++ b/src/test/linters/lint.unit.test.ts @@ -656,13 +656,16 @@ suite('Linting Scenarios', () => { // Note that these aren't actually unit tests. Instead they are // integration tests with heavy usage of mocks. - let isExtensionInstalledStub: sinon.SinonStub; + let isExtensionEnabledStub: sinon.SinonStub; + let isExtensionDisabledStub: sinon.SinonStub; let doNotShowPromptStateStub: sinon.SinonStub; let persistentState: TypeMoq.IMock>; setup(() => { - isExtensionInstalledStub = sinon.stub(promptApis, 'isExtensionInstalled'); + isExtensionEnabledStub = sinon.stub(promptApis, 'isExtensionEnabled'); + isExtensionDisabledStub = sinon.stub(promptApis, 'isExtensionDisabled'); // For these tests we assume that linter extensions are not installed. - isExtensionInstalledStub.returns(false); + isExtensionEnabledStub.returns(false); + isExtensionDisabledStub.returns(false); persistentState = TypeMoq.Mock.ofType>(); persistentState.setup((p) => p.value).returns(() => true); diff --git a/src/test/linters/prompts/flake8Prompt.unit.test.ts b/src/test/linters/prompts/flake8Prompt.unit.test.ts index 2764617b7d6a..7bbe52ae6d96 100644 --- a/src/test/linters/prompts/flake8Prompt.unit.test.ts +++ b/src/test/linters/prompts/flake8Prompt.unit.test.ts @@ -15,7 +15,8 @@ import { Flake8ExtensionPrompt, FLAKE8_EXTENSION } from '../../../client/linters import { IToolsExtensionPrompt } from '../../../client/linters/prompts/types'; suite('Flake8 Extension prompt tests', () => { - let isExtensionInstalledStub: sinon.SinonStub; + let isExtensionEnabledStub: sinon.SinonStub; + let isExtensionDisabledStub: sinon.SinonStub; let doNotShowPromptStateStub: sinon.SinonStub; let inToolsExtensionsExperimentStub: sinon.SinonStub; let showInformationMessageStub: sinon.SinonStub; @@ -26,7 +27,8 @@ suite('Flake8 Extension prompt tests', () => { let prompt: IToolsExtensionPrompt; setup(() => { - isExtensionInstalledStub = sinon.stub(promptCommons, 'isExtensionInstalled'); + isExtensionEnabledStub = sinon.stub(promptCommons, 'isExtensionEnabled'); + isExtensionDisabledStub = sinon.stub(promptCommons, 'isExtensionDisabled'); doNotShowPromptStateStub = sinon.stub(promptCommons, 'doNotShowPromptState'); inToolsExtensionsExperimentStub = sinon.stub(promptCommons, 'inToolsExtensionsExperiment'); showInformationMessageStub = sinon.stub(windowsApis, 'showInformationMessage'); @@ -46,14 +48,23 @@ suite('Flake8 Extension prompt tests', () => { sinon.restore(); }); - test('Extension already installed', async () => { - isExtensionInstalledStub.returns(true); + test('Extension already installed and enabled', async () => { + isExtensionEnabledStub.returns(true); + + assert.isTrue(await prompt.showPrompt()); + }); + + test('Extension already installed, but disabled', async () => { + isExtensionEnabledStub.returns(false); + isExtensionDisabledStub.returns(true); assert.isTrue(await prompt.showPrompt()); }); test('Test do not show again persistent state', async () => { - isExtensionInstalledStub.returns(false); + isExtensionEnabledStub.returns(false); + isExtensionDisabledStub.returns(false); + doNotState.setup((d) => d.value).returns(() => true); doNotShowPromptStateStub.returns(doNotState.object); @@ -61,7 +72,9 @@ suite('Flake8 Extension prompt tests', () => { }); test('User not in experiment', async () => { - isExtensionInstalledStub.returns(false); + isExtensionEnabledStub.returns(false); + isExtensionDisabledStub.returns(false); + doNotState.setup((d) => d.value).returns(() => false); doNotShowPromptStateStub.returns(doNotState.object); inToolsExtensionsExperimentStub.resolves(false); @@ -70,7 +83,9 @@ suite('Flake8 Extension prompt tests', () => { }); test('User selected: install extension (insiders)', async () => { - isExtensionInstalledStub.returns(false); + isExtensionEnabledStub.returns(false); + isExtensionDisabledStub.returns(false); + doNotState.setup((d) => d.value).returns(() => false); doNotShowPromptStateStub.returns(doNotState.object); inToolsExtensionsExperimentStub.resolves(true); @@ -87,7 +102,9 @@ suite('Flake8 Extension prompt tests', () => { }); test('User selected: install extension (stable)', async () => { - isExtensionInstalledStub.returns(false); + isExtensionEnabledStub.returns(false); + isExtensionDisabledStub.returns(false); + doNotState.setup((d) => d.value).returns(() => false); doNotShowPromptStateStub.returns(doNotState.object); inToolsExtensionsExperimentStub.resolves(true); @@ -104,7 +121,9 @@ suite('Flake8 Extension prompt tests', () => { }); test('User selected: do not show again', async () => { - isExtensionInstalledStub.returns(false); + isExtensionEnabledStub.returns(false); + isExtensionDisabledStub.returns(false); + doNotState.setup((d) => d.value).returns(() => false); doNotShowPromptStateStub.returns(doNotState.object); inToolsExtensionsExperimentStub.resolves(true); @@ -120,7 +139,9 @@ suite('Flake8 Extension prompt tests', () => { }); test('User selected: close', async () => { - isExtensionInstalledStub.returns(false); + isExtensionEnabledStub.returns(false); + isExtensionDisabledStub.returns(false); + doNotState.setup((d) => d.value).returns(() => false); doNotShowPromptStateStub.returns(doNotState.object); inToolsExtensionsExperimentStub.resolves(true); diff --git a/src/test/linters/prompts/pylintPrompt.unit.test.ts b/src/test/linters/prompts/pylintPrompt.unit.test.ts index 7cfcc246ff16..65b579f258af 100644 --- a/src/test/linters/prompts/pylintPrompt.unit.test.ts +++ b/src/test/linters/prompts/pylintPrompt.unit.test.ts @@ -15,7 +15,8 @@ import { PylintExtensionPrompt, PYLINT_EXTENSION } from '../../../client/linters import { IToolsExtensionPrompt } from '../../../client/linters/prompts/types'; suite('Pylint Extension prompt tests', () => { - let isExtensionInstalledStub: sinon.SinonStub; + let isExtensionEnabledStub: sinon.SinonStub; + let isExtensionDisabledStub: sinon.SinonStub; let doNotShowPromptStateStub: sinon.SinonStub; let inToolsExtensionsExperimentStub: sinon.SinonStub; let showInformationMessageStub: sinon.SinonStub; @@ -26,7 +27,8 @@ suite('Pylint Extension prompt tests', () => { let prompt: IToolsExtensionPrompt; setup(() => { - isExtensionInstalledStub = sinon.stub(promptCommons, 'isExtensionInstalled'); + isExtensionEnabledStub = sinon.stub(promptCommons, 'isExtensionEnabled'); + isExtensionDisabledStub = sinon.stub(promptCommons, 'isExtensionDisabled'); doNotShowPromptStateStub = sinon.stub(promptCommons, 'doNotShowPromptState'); inToolsExtensionsExperimentStub = sinon.stub(promptCommons, 'inToolsExtensionsExperiment'); showInformationMessageStub = sinon.stub(windowsApis, 'showInformationMessage'); @@ -46,22 +48,23 @@ suite('Pylint Extension prompt tests', () => { sinon.restore(); }); - test('Extension already installed', async () => { - isExtensionInstalledStub.returns(true); + test('Extension already installed and enabled', async () => { + isExtensionEnabledStub.returns(true); assert.isTrue(await prompt.showPrompt()); }); - test('Test do not show again persistent state', async () => { - isExtensionInstalledStub.returns(false); - doNotState.setup((d) => d.value).returns(() => true); - doNotShowPromptStateStub.returns(doNotState.object); + test('Extension already installed, but disabled', async () => { + isExtensionEnabledStub.returns(false); + isExtensionDisabledStub.returns(true); - assert.isFalse(await prompt.showPrompt()); + assert.isTrue(await prompt.showPrompt()); }); test('User not in experiment', async () => { - isExtensionInstalledStub.returns(false); + isExtensionEnabledStub.returns(false); + isExtensionDisabledStub.returns(false); + doNotState.setup((d) => d.value).returns(() => false); doNotShowPromptStateStub.returns(doNotState.object); inToolsExtensionsExperimentStub.resolves(false); @@ -70,7 +73,9 @@ suite('Pylint Extension prompt tests', () => { }); test('User selected: install extension (insiders)', async () => { - isExtensionInstalledStub.returns(false); + isExtensionEnabledStub.returns(false); + isExtensionDisabledStub.returns(false); + doNotState.setup((d) => d.value).returns(() => false); doNotShowPromptStateStub.returns(doNotState.object); inToolsExtensionsExperimentStub.resolves(true); @@ -87,7 +92,9 @@ suite('Pylint Extension prompt tests', () => { }); test('User selected: install extension (stable)', async () => { - isExtensionInstalledStub.returns(false); + isExtensionEnabledStub.returns(false); + isExtensionDisabledStub.returns(false); + doNotState.setup((d) => d.value).returns(() => false); doNotShowPromptStateStub.returns(doNotState.object); inToolsExtensionsExperimentStub.resolves(true); @@ -104,7 +111,9 @@ suite('Pylint Extension prompt tests', () => { }); test('User selected: do not show again', async () => { - isExtensionInstalledStub.returns(false); + isExtensionEnabledStub.returns(false); + isExtensionDisabledStub.returns(false); + doNotState.setup((d) => d.value).returns(() => false); doNotShowPromptStateStub.returns(doNotState.object); inToolsExtensionsExperimentStub.resolves(true); @@ -120,7 +129,9 @@ suite('Pylint Extension prompt tests', () => { }); test('User selected: close', async () => { - isExtensionInstalledStub.returns(false); + isExtensionEnabledStub.returns(false); + isExtensionDisabledStub.returns(false); + doNotState.setup((d) => d.value).returns(() => false); doNotShowPromptStateStub.returns(doNotState.object); inToolsExtensionsExperimentStub.resolves(true); From f99b8b68d0c0e0def866fa8c130af035c95020a1 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Mon, 6 Feb 2023 12:16:51 -0800 Subject: [PATCH 4/4] Fix tests. --- src/test/linters/lint.args.test.ts | 13 ++++++++++++- src/test/linters/pylint.test.ts | 7 ++++++- src/test/linters/pylint.unit.test.ts | 5 ++++- 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/src/test/linters/lint.args.test.ts b/src/test/linters/lint.args.test.ts index b12682ea21c6..2c32a73052bf 100644 --- a/src/test/linters/lint.args.test.ts +++ b/src/test/linters/lint.args.test.ts @@ -11,7 +11,13 @@ import { CancellationTokenSource, TextDocument, Uri, WorkspaceFolder } from 'vsc import { IDocumentManager, IWorkspaceService } from '../../client/common/application/types'; import '../../client/common/extensions'; import { IFileSystem, IPlatformService } from '../../client/common/platform/types'; -import { IConfigurationService, IInstaller, ILintingSettings, IPythonSettings } from '../../client/common/types'; +import { + IConfigurationService, + IExtensions, + IInstaller, + ILintingSettings, + IPythonSettings, +} from '../../client/common/types'; import { IInterpreterAutoSelectionService, IInterpreterAutoSelectionProxyService, @@ -53,6 +59,7 @@ suite('Linting - Arguments', () => { let serviceContainer: ServiceContainer; let document: TypeMoq.IMock; let workspaceService: TypeMoq.IMock; + let extensionsService: TypeMoq.IMock; const cancellationToken = new CancellationTokenSource().token; suiteSetup(initialize); setup(async () => { @@ -122,6 +129,10 @@ suite('Linting - Arguments', () => { const platformService = TypeMoq.Mock.ofType(); serviceManager.addSingletonInstance(IPlatformService, platformService.object); + extensionsService = TypeMoq.Mock.ofType(); + extensionsService.setup((e) => e.getExtension(TypeMoq.It.isAny())).returns(() => undefined); + serviceManager.addSingletonInstance(IExtensions, extensionsService.object); + lm = new LinterManager(configService.object); serviceManager.addSingletonInstance(ILinterManager, lm); document = TypeMoq.Mock.ofType(); diff --git a/src/test/linters/pylint.test.ts b/src/test/linters/pylint.test.ts index dd97931d9b93..e1cec249c662 100644 --- a/src/test/linters/pylint.test.ts +++ b/src/test/linters/pylint.test.ts @@ -18,7 +18,7 @@ import { LanguageServerType } from '../../client/activation/types'; import { IWorkspaceService } from '../../client/common/application/types'; import { IFileSystem, IPlatformService } from '../../client/common/platform/types'; import { IPythonToolExecutionService } from '../../client/common/process/types'; -import { IConfigurationService, IInstaller, IPythonSettings } from '../../client/common/types'; +import { IConfigurationService, IExtensions, IInstaller, IPythonSettings } from '../../client/common/types'; import { IInterpreterAutoSelectionService, IInterpreterAutoSelectionProxyService, @@ -40,6 +40,7 @@ suite('Linting - Pylint', () => { let workspaceConfig: TypeMoq.IMock; let pythonSettings: TypeMoq.IMock; let serviceContainer: ServiceContainer; + let extensionsService: TypeMoq.IMock; setup(() => { fileSystem = TypeMoq.Mock.ofType(); @@ -50,6 +51,9 @@ suite('Linting - Pylint', () => { platformService = TypeMoq.Mock.ofType(); platformService.setup((x) => x.isWindows).returns(() => false); + extensionsService = TypeMoq.Mock.ofType(); + extensionsService.setup((e) => e.getExtension(TypeMoq.It.isAny())).returns(() => undefined); + workspace = TypeMoq.Mock.ofType(); execService = TypeMoq.Mock.ofType(); @@ -72,6 +76,7 @@ suite('Linting - Pylint', () => { IInterpreterAutoSelectionProxyService, MockAutoSelectionService, ); + serviceManager.addSingletonInstance(IExtensions, extensionsService.object); pythonSettings = TypeMoq.Mock.ofType(); pythonSettings.setup((p) => p.languageServer).returns(() => LanguageServerType.Jedi); diff --git a/src/test/linters/pylint.unit.test.ts b/src/test/linters/pylint.unit.test.ts index d256e025df43..ee6954e870a5 100644 --- a/src/test/linters/pylint.unit.test.ts +++ b/src/test/linters/pylint.unit.test.ts @@ -8,7 +8,7 @@ import * as TypeMoq from 'typemoq'; import * as vscode from 'vscode'; import { IWorkspaceService } from '../../client/common/application/types'; import { IFileSystem, IPlatformService } from '../../client/common/platform/types'; -import { IConfigurationService, IPythonSettings } from '../../client/common/types'; +import { IConfigurationService, IExtensions, IPythonSettings } from '../../client/common/types'; import { IServiceContainer } from '../../client/ioc/types'; import { IToolsExtensionPrompt } from '../../client/linters/prompts/types'; import { Pylint } from '../../client/linters/pylint'; @@ -22,6 +22,7 @@ suite('Pylint - Function runLinter()', () => { let manager: TypeMoq.IMock; let _info: TypeMoq.IMock; let platformService: TypeMoq.IMock; + let extensionsService: TypeMoq.IMock; let run: sinon.SinonStub; let parseMessagesSeverity: sinon.SinonStub; let extensionPrompt: TypeMoq.IMock; @@ -77,6 +78,7 @@ suite('Pylint - Function runLinter()', () => { serviceContainer = TypeMoq.Mock.ofType(); workspaceService = TypeMoq.Mock.ofType(); configService = TypeMoq.Mock.ofType(); + extensionsService = TypeMoq.Mock.ofType(); manager = TypeMoq.Mock.ofType(); serviceContainer.setup((c) => c.get(TypeMoq.It.isValue(ILinterManager))).returns(() => manager.object); serviceContainer @@ -89,6 +91,7 @@ suite('Pylint - Function runLinter()', () => { serviceContainer .setup((c) => c.get(TypeMoq.It.isValue(IPlatformService))) .returns(() => platformService.object); + serviceContainer.setup((c) => c.get(TypeMoq.It.isValue(IExtensions))).returns(() => extensionsService.object); fileSystem = TypeMoq.Mock.ofType(); fileSystem .setup((x) => x.arePathsSame(TypeMoq.It.isAnyString(), TypeMoq.It.isAnyString()))