From e134ad04a54e811c33382a82c9bc9871dcb0aa17 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Fri, 1 Jul 2022 16:58:56 -0700 Subject: [PATCH 01/10] Improve prompts in case no Python is installed or an invalid interpreter is selected --- src/client/application/diagnostics/base.ts | 6 +- .../diagnostics/checks/pythonInterpreter.ts | 91 ++++++------------- .../application/diagnostics/constants.ts | 2 +- src/client/application/diagnostics/types.ts | 6 ++ src/client/telemetry/constants.ts | 1 - src/client/telemetry/index.ts | 18 ---- .../checks/pythonInterpreter.unit.test.ts | 15 +-- 7 files changed, 44 insertions(+), 95 deletions(-) diff --git a/src/client/application/diagnostics/base.ts b/src/client/application/diagnostics/base.ts index abc70e210e1f..61c65935ddd8 100644 --- a/src/client/application/diagnostics/base.ts +++ b/src/client/application/diagnostics/base.ts @@ -7,6 +7,7 @@ import { injectable, unmanaged } from 'inversify'; import { DiagnosticSeverity } from 'vscode'; import { IWorkspaceService } from '../../common/application/types'; import { IDisposable, IDisposableRegistry, Resource } from '../../common/types'; +import { asyncFilter } from '../../common/utils/arrayUtils'; import { IServiceContainer } from '../../ioc/types'; import { sendTelemetryEvent } from '../../telemetry'; import { EventName } from '../../telemetry/constants'; @@ -48,10 +49,13 @@ export abstract class BaseDiagnosticsService implements IDiagnosticsService, IDi if (diagnostics.length === 0) { return; } - const diagnosticsToHandle = diagnostics.filter((item) => { + const diagnosticsToHandle = await asyncFilter(diagnostics, async (item) => { if (item.invokeHandler && item.invokeHandler === 'always') { return true; } + if (!(await this.canHandle(item))) { + return false; + } const key = this.getDiagnosticsKey(item); if (BaseDiagnosticsService.handledDiagnosticCodeKeys.indexOf(key) !== -1) { return false; diff --git a/src/client/application/diagnostics/checks/pythonInterpreter.ts b/src/client/application/diagnostics/checks/pythonInterpreter.ts index 89d63c230d92..86bec5661a1f 100644 --- a/src/client/application/diagnostics/checks/pythonInterpreter.ts +++ b/src/client/application/diagnostics/checks/pythonInterpreter.ts @@ -9,39 +9,30 @@ import * as nls from 'vscode-nls'; import { IDisposableRegistry, Resource } from '../../../common/types'; import { IInterpreterService } from '../../../interpreter/contracts'; import { IServiceContainer } from '../../../ioc/types'; -import { sendTelemetryEvent } from '../../../telemetry'; -import { EventName } from '../../../telemetry/constants'; import { BaseDiagnostic, BaseDiagnosticsService } from '../base'; import { IDiagnosticsCommandFactory } from '../commands/types'; import { DiagnosticCodes } from '../constants'; import { DiagnosticCommandPromptHandlerServiceId, MessageCommandPrompt } from '../promptHandler'; -import { - DiagnosticScope, - IDiagnostic, - IDiagnosticCommand, - IDiagnosticHandlerService, - IDiagnosticMessageOnCloseHandler, -} from '../types'; +import { DiagnosticScope, IDiagnostic, IDiagnosticCommand, IDiagnosticHandlerService } from '../types'; import { Common } from '../../../common/utils/localize'; +import { Commands } from '../../../common/constants'; const localize: nls.LocalizeFunc = nls.loadMessageBundle(); const messages = { [DiagnosticCodes.NoPythonInterpretersDiagnostic]: localize( 'DiagnosticCodes.NoPythonInterpretersDiagnostic', - 'Python is not installed. Please download and install Python before using the extension.', + 'No Python interpreter is selected. You need to select a Python interpreter to enable features such as IntelliSense, linting, and debugging.', ), - [DiagnosticCodes.NoCurrentlySelectedPythonInterpreterDiagnostic]: localize( + [DiagnosticCodes.InvalidPythonInterpreterDiagnostic]: localize( 'DiagnosticCodes.NoCurrentlySelectedPythonInterpreterDiagnostic', - 'No Python interpreter is selected. You need to select a Python interpreter to enable features such as IntelliSense, linting, and debugging.', + 'An Invalid Python interpreter is selected, please try changing it to enable features such as IntelliSense, linting, and debugging.', ), }; export class InvalidPythonInterpreterDiagnostic extends BaseDiagnostic { constructor( - code: - | DiagnosticCodes.NoPythonInterpretersDiagnostic - | DiagnosticCodes.NoCurrentlySelectedPythonInterpreterDiagnostic, + code: DiagnosticCodes.NoPythonInterpretersDiagnostic | DiagnosticCodes.InvalidPythonInterpreterDiagnostic, resource: Resource, ) { super(code, messages[code], DiagnosticSeverity.Error, DiagnosticScope.WorkspaceFolder, resource); @@ -57,10 +48,7 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService { @inject(IDisposableRegistry) disposableRegistry: IDisposableRegistry, ) { super( - [ - DiagnosticCodes.NoPythonInterpretersDiagnostic, - DiagnosticCodes.NoCurrentlySelectedPythonInterpreterDiagnostic, - ], + [DiagnosticCodes.NoPythonInterpretersDiagnostic, DiagnosticCodes.InvalidPythonInterpreterDiagnostic], serviceContainer, disposableRegistry, false, @@ -78,16 +66,21 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService { const currentInterpreter = await interpreterService.getActiveInterpreter(resource); if (!currentInterpreter) { return [ - new InvalidPythonInterpreterDiagnostic( - DiagnosticCodes.NoCurrentlySelectedPythonInterpreterDiagnostic, - resource, - ), + new InvalidPythonInterpreterDiagnostic(DiagnosticCodes.InvalidPythonInterpreterDiagnostic, resource), ]; } - return []; } + public async validateInterpreterPathInSettings(resource: Resource): Promise { + const diagnostics = await this.diagnose(resource); + if (!diagnostics.length) { + return true; + } + this.handle(diagnostics).ignoreErrors(); + return false; + } + protected async onHandle(diagnostics: IDiagnostic[]): Promise { if (diagnostics.length === 0) { return; @@ -102,51 +95,21 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService { return; } const commandPrompts = this.getCommandPrompts(diagnostic); - const onClose = getOnCloseHandler(diagnostic); - await messageService.handle(diagnostic, { commandPrompts, message: diagnostic.message, onClose }); + await messageService.handle(diagnostic, { commandPrompts, message: diagnostic.message }); }), ); } private getCommandPrompts(diagnostic: IDiagnostic): { prompt: string; command?: IDiagnosticCommand }[] { const commandFactory = this.serviceContainer.get(IDiagnosticsCommandFactory); - switch (diagnostic.code) { - case DiagnosticCodes.NoPythonInterpretersDiagnostic: { - return [ - { - prompt: Common.download, - command: commandFactory.createCommand(diagnostic, { - type: 'launch', - options: 'https://www.python.org/downloads', - }), - }, - ]; - } - case DiagnosticCodes.NoCurrentlySelectedPythonInterpreterDiagnostic: { - return [ - { - prompt: Common.selectPythonInterpreter, - command: commandFactory.createCommand(diagnostic, { - type: 'executeVSCCommand', - options: 'python.setInterpreter', - }), - }, - ]; - } - default: { - throw new Error("Invalid diagnostic for 'InvalidPythonInterpreterService'"); - } - } - } -} - -function getOnCloseHandler(diagnostic: IDiagnostic): IDiagnosticMessageOnCloseHandler | undefined { - if (diagnostic.code === DiagnosticCodes.NoPythonInterpretersDiagnostic) { - return (response?: string) => { - sendTelemetryEvent(EventName.PYTHON_NOT_INSTALLED_PROMPT, undefined, { - selection: response ? 'Download' : 'Ignore', - }); - }; + return [ + { + prompt: Common.selectPythonInterpreter, + command: commandFactory.createCommand(diagnostic, { + type: 'executeVSCCommand', + options: Commands.Set_Interpreter, + }), + }, + ]; } - return undefined; } diff --git a/src/client/application/diagnostics/constants.ts b/src/client/application/diagnostics/constants.ts index 850b7dab3855..9fdd6ff13723 100644 --- a/src/client/application/diagnostics/constants.ts +++ b/src/client/application/diagnostics/constants.ts @@ -11,7 +11,7 @@ export enum DiagnosticCodes { InvalidPythonPathInDebuggerSettingsDiagnostic = 'InvalidPythonPathInDebuggerSettingsDiagnostic', InvalidPythonPathInDebuggerLaunchDiagnostic = 'InvalidPythonPathInDebuggerLaunchDiagnostic', EnvironmentActivationInPowerShellWithBatchFilesNotSupportedDiagnostic = 'EnvironmentActivationInPowerShellWithBatchFilesNotSupportedDiagnostic', - NoCurrentlySelectedPythonInterpreterDiagnostic = 'InvalidPythonInterpreterDiagnostic', + InvalidPythonInterpreterDiagnostic = 'InvalidPythonInterpreterDiagnostic', LSNotSupportedDiagnostic = 'LSNotSupportedDiagnostic', PythonPathDeprecatedDiagnostic = 'PythonPathDeprecatedDiagnostic', JustMyCodeDiagnostic = 'JustMyCodeDiagnostic', diff --git a/src/client/application/diagnostics/types.ts b/src/client/application/diagnostics/types.ts index 343ba0f02cd3..ced9930c81ab 100644 --- a/src/client/application/diagnostics/types.ts +++ b/src/client/application/diagnostics/types.ts @@ -53,6 +53,12 @@ export interface IDiagnosticCommand { export type IDiagnosticMessageOnCloseHandler = (response?: string) => void; +export const IInvalidPythonPathInSettings = Symbol('IInvalidPythonPathInSettings'); + +export interface IInvalidPythonPathInSettings extends IDiagnosticsService { + validateInterpreterPathInSettings(resource: Resource): Promise; +} + export const IInvalidPythonPathInDebuggerService = Symbol('IInvalidPythonPathInDebuggerService'); export interface IInvalidPythonPathInDebuggerService extends IDiagnosticsService { diff --git a/src/client/telemetry/constants.ts b/src/client/telemetry/constants.ts index 2ab6c8a8a3ba..45706d868ae5 100644 --- a/src/client/telemetry/constants.ts +++ b/src/client/telemetry/constants.ts @@ -26,7 +26,6 @@ export enum EventName { PYTHON_INTERPRETER_ACTIVATION_FOR_TERMINAL = 'PYTHON_INTERPRETER_ACTIVATION_FOR_TERMINAL', TERMINAL_SHELL_IDENTIFICATION = 'TERMINAL_SHELL_IDENTIFICATION', PYTHON_INTERPRETER_ACTIVATE_ENVIRONMENT_PROMPT = 'PYTHON_INTERPRETER_ACTIVATE_ENVIRONMENT_PROMPT', - PYTHON_NOT_INSTALLED_PROMPT = 'PYTHON_NOT_INSTALLED_PROMPT', CONDA_INHERIT_ENV_PROMPT = 'CONDA_INHERIT_ENV_PROMPT', ENVFILE_VARIABLE_SUBSTITUTION = 'ENVFILE_VARIABLE_SUBSTITUTION', ENVFILE_WORKSPACE = 'ENVFILE_WORKSPACE', diff --git a/src/client/telemetry/index.ts b/src/client/telemetry/index.ts index 7f056894f79e..017f41a72ed3 100644 --- a/src/client/telemetry/index.ts +++ b/src/client/telemetry/index.ts @@ -1319,24 +1319,6 @@ export interface IEventNamePropertyMapping { */ selection: 'Yes' | 'No' | 'Ignore' | undefined; }; - /** - * Telemetry event sent with details when the user clicks a button in the "Python is not installed" prompt. - * * `Prompt message` :- 'Python is not installed. Please download and install Python before using the extension.' - */ - /* __GDPR__ - "python_not_installed_prompt" : { - "selection" : { "classification": "SystemMetaData", "purpose": "FeatureInsight", "owner": "karrtikr" } - } - */ - [EventName.PYTHON_NOT_INSTALLED_PROMPT]: { - /** - * `Download` When the 'Download' option is clicked - * `Ignore` When the prompt is dismissed - * - * @type {('Download' | 'Ignore' | undefined)} - */ - selection: 'Download' | 'Ignore' | undefined; - }; /** * Telemetry event sent when the experiments service is initialized for the first time. */ diff --git a/src/test/application/diagnostics/checks/pythonInterpreter.unit.test.ts b/src/test/application/diagnostics/checks/pythonInterpreter.unit.test.ts index 8fcf78de34df..f2a82982e564 100644 --- a/src/test/application/diagnostics/checks/pythonInterpreter.unit.test.ts +++ b/src/test/application/diagnostics/checks/pythonInterpreter.unit.test.ts @@ -94,7 +94,7 @@ suite('Application Diagnostics - Checks Python Interpreter', () => { test('Can handle InvalidPythonPathInterpreter diagnostics', async () => { for (const code of [ DiagnosticCodes.NoPythonInterpretersDiagnostic, - DiagnosticCodes.NoCurrentlySelectedPythonInterpreterDiagnostic, + DiagnosticCodes.InvalidPythonInterpreterDiagnostic, ]) { const diagnostic = typemoq.Mock.ofType(); diagnostic @@ -148,12 +148,7 @@ suite('Application Diagnostics - Checks Python Interpreter', () => { const diagnostics = await diagnosticService.diagnose(undefined); expect(diagnostics).to.be.deep.equal( - [ - new InvalidPythonInterpreterDiagnostic( - DiagnosticCodes.NoCurrentlySelectedPythonInterpreterDiagnostic, - undefined, - ), - ], + [new InvalidPythonInterpreterDiagnostic(DiagnosticCodes.InvalidPythonInterpreterDiagnostic, undefined)], 'not the same', ); settings.verifyAll(); @@ -208,7 +203,7 @@ suite('Application Diagnostics - Checks Python Interpreter', () => { }); test('Handling no currently selected interpreter diagnostic should show select interpreter message', async () => { const diagnostic = new InvalidPythonInterpreterDiagnostic( - DiagnosticCodes.NoCurrentlySelectedPythonInterpreterDiagnostic, + DiagnosticCodes.InvalidPythonInterpreterDiagnostic, undefined, ); const cmd = ({} as any) as IDiagnosticCommand; @@ -242,7 +237,7 @@ suite('Application Diagnostics - Checks Python Interpreter', () => { }); test('Handling no interpreters diagnostic should return select interpreter cmd', async () => { const diagnostic = new InvalidPythonInterpreterDiagnostic( - DiagnosticCodes.NoCurrentlySelectedPythonInterpreterDiagnostic, + DiagnosticCodes.InvalidPythonInterpreterDiagnostic, undefined, ); const cmd = ({} as any) as IDiagnosticCommand; @@ -302,7 +297,7 @@ suite('Application Diagnostics - Checks Python Interpreter', () => { }); test('Handling an unsupported diagnostic code should not show a message nor return a command', async () => { const diagnostic = new InvalidPythonInterpreterDiagnostic( - DiagnosticCodes.NoCurrentlySelectedPythonInterpreterDiagnostic, + DiagnosticCodes.InvalidPythonInterpreterDiagnostic, undefined, ); const cmd = ({} as any) as IDiagnosticCommand; From 4c498d82e98297b7adc005c78a40950d39ae3588 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Fri, 1 Jul 2022 17:17:57 -0700 Subject: [PATCH 02/10] Specify folder name in case of multiroot scenarios --- .../diagnostics/checks/pythonInterpreter.ts | 36 ++++++++++++++++--- 1 file changed, 32 insertions(+), 4 deletions(-) diff --git a/src/client/application/diagnostics/checks/pythonInterpreter.ts b/src/client/application/diagnostics/checks/pythonInterpreter.ts index 86bec5661a1f..a2c77e461cd5 100644 --- a/src/client/application/diagnostics/checks/pythonInterpreter.ts +++ b/src/client/application/diagnostics/checks/pythonInterpreter.ts @@ -6,6 +6,7 @@ import { inject, injectable } from 'inversify'; import { DiagnosticSeverity } from 'vscode'; import '../../../common/extensions'; import * as nls from 'vscode-nls'; +import * as path from 'path'; import { IDisposableRegistry, Resource } from '../../../common/types'; import { IInterpreterService } from '../../../interpreter/contracts'; import { IServiceContainer } from '../../../ioc/types'; @@ -16,6 +17,7 @@ import { DiagnosticCommandPromptHandlerServiceId, MessageCommandPrompt } from '. import { DiagnosticScope, IDiagnostic, IDiagnosticCommand, IDiagnosticHandlerService } from '../types'; import { Common } from '../../../common/utils/localize'; import { Commands } from '../../../common/constants'; +import { IWorkspaceService } from '../../../common/application/types'; const localize: nls.LocalizeFunc = nls.loadMessageBundle(); @@ -26,7 +28,7 @@ const messages = { ), [DiagnosticCodes.InvalidPythonInterpreterDiagnostic]: localize( 'DiagnosticCodes.NoCurrentlySelectedPythonInterpreterDiagnostic', - 'An Invalid Python interpreter is selected, please try changing it to enable features such as IntelliSense, linting, and debugging.', + 'An Invalid Python interpreter is selected{0}, please try changing it to enable features such as IntelliSense, linting, and debugging.', ), }; @@ -34,8 +36,23 @@ export class InvalidPythonInterpreterDiagnostic extends BaseDiagnostic { constructor( code: DiagnosticCodes.NoPythonInterpretersDiagnostic | DiagnosticCodes.InvalidPythonInterpreterDiagnostic, resource: Resource, + workspaceService: IWorkspaceService, ) { - super(code, messages[code], DiagnosticSeverity.Error, DiagnosticScope.WorkspaceFolder, resource); + let formatArg = ''; + if (workspaceService.workspaceFile) { + // Specify folder name in case of multiroot scenarios + const folder = workspaceService.getWorkspaceFolder(resource); + if (folder) { + formatArg = ` for ${path.basename(folder.uri.fsPath)}`; + } + } + super( + code, + messages[code].format(formatArg), + DiagnosticSeverity.Error, + DiagnosticScope.WorkspaceFolder, + resource, + ); } } @@ -56,17 +73,28 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService { } public async diagnose(resource: Resource): Promise { + const workspaceService = this.serviceContainer.get(IWorkspaceService); const interpreterService = this.serviceContainer.get(IInterpreterService); const hasInterpreters = await interpreterService.hasInterpreters(); if (!hasInterpreters) { - return [new InvalidPythonInterpreterDiagnostic(DiagnosticCodes.NoPythonInterpretersDiagnostic, resource)]; + return [ + new InvalidPythonInterpreterDiagnostic( + DiagnosticCodes.NoPythonInterpretersDiagnostic, + resource, + workspaceService, + ), + ]; } const currentInterpreter = await interpreterService.getActiveInterpreter(resource); if (!currentInterpreter) { return [ - new InvalidPythonInterpreterDiagnostic(DiagnosticCodes.InvalidPythonInterpreterDiagnostic, resource), + new InvalidPythonInterpreterDiagnostic( + DiagnosticCodes.InvalidPythonInterpreterDiagnostic, + resource, + workspaceService, + ), ]; } return []; From a27ce604ab32c5f630b13596254c79b658f28b5f Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Mon, 11 Jul 2022 13:36:27 -0700 Subject: [PATCH 03/10] Fix tests --- src/client/application/diagnostics/base.ts | 8 +-- .../checks/invalidLaunchJsonDebugger.ts | 7 +-- .../checks/invalidPythonPathInDebugger.ts | 10 +++- .../checks/powerShellActivation.ts | 1 + .../diagnostics/checks/pythonInterpreter.ts | 2 + .../checks/pythonInterpreter.unit.test.ts | 56 +++++++++++++------ 6 files changed, 58 insertions(+), 26 deletions(-) diff --git a/src/client/application/diagnostics/base.ts b/src/client/application/diagnostics/base.ts index 61c65935ddd8..db8081172338 100644 --- a/src/client/application/diagnostics/base.ts +++ b/src/client/application/diagnostics/base.ts @@ -22,8 +22,8 @@ export abstract class BaseDiagnostic implements IDiagnostic { public readonly severity: DiagnosticSeverity, public readonly scope: DiagnosticScope, public readonly resource: Resource, - public readonly invokeHandler: 'always' | 'default' = 'default', public readonly shouldShowPrompt = true, + public readonly invokeHandler: 'always' | 'default' = 'default', ) {} } @@ -50,12 +50,12 @@ export abstract class BaseDiagnosticsService implements IDiagnosticsService, IDi return; } const diagnosticsToHandle = await asyncFilter(diagnostics, async (item) => { - if (item.invokeHandler && item.invokeHandler === 'always') { - return true; - } if (!(await this.canHandle(item))) { return false; } + if (item.invokeHandler && item.invokeHandler === 'always') { + return true; + } const key = this.getDiagnosticsKey(item); if (BaseDiagnosticsService.handledDiagnosticCodeKeys.indexOf(key) !== -1) { return false; diff --git a/src/client/application/diagnostics/checks/invalidLaunchJsonDebugger.ts b/src/client/application/diagnostics/checks/invalidLaunchJsonDebugger.ts index d6757d5a844e..7a955271f3a3 100644 --- a/src/client/application/diagnostics/checks/invalidLaunchJsonDebugger.ts +++ b/src/client/application/diagnostics/checks/invalidLaunchJsonDebugger.ts @@ -39,7 +39,6 @@ export class InvalidLaunchJsonDebuggerDiagnostic extends BaseDiagnostic { DiagnosticSeverity.Error, DiagnosticScope.WorkspaceFolder, resource, - 'always', shouldShowPrompt, ); } @@ -131,9 +130,9 @@ export class InvalidLaunchJsonDebuggerService extends BaseDiagnosticsService { } private async handleDiagnostic(diagnostic: IDiagnostic): Promise { - if (!this.canHandle(diagnostic)) { - return; - } + // if (!this.canHandle(diagnostic)) { + // return; + // } if (!diagnostic.shouldShowPrompt) { await this.fixLaunchJson(diagnostic.code); return; diff --git a/src/client/application/diagnostics/checks/invalidPythonPathInDebugger.ts b/src/client/application/diagnostics/checks/invalidPythonPathInDebugger.ts index add725eac9c0..f08c09956838 100644 --- a/src/client/application/diagnostics/checks/invalidPythonPathInDebugger.ts +++ b/src/client/application/diagnostics/checks/invalidPythonPathInDebugger.ts @@ -38,7 +38,15 @@ class InvalidPythonPathInDebuggerDiagnostic extends BaseDiagnostic { | DiagnosticCodes.InvalidPythonPathInDebuggerSettingsDiagnostic, resource: Resource, ) { - super(code, messages[code], DiagnosticSeverity.Error, DiagnosticScope.WorkspaceFolder, resource, 'always'); + super( + code, + messages[code], + DiagnosticSeverity.Error, + DiagnosticScope.WorkspaceFolder, + resource, + undefined, + 'always', + ); } } diff --git a/src/client/application/diagnostics/checks/powerShellActivation.ts b/src/client/application/diagnostics/checks/powerShellActivation.ts index f4d445ec042a..4ffdf21a9173 100644 --- a/src/client/application/diagnostics/checks/powerShellActivation.ts +++ b/src/client/application/diagnostics/checks/powerShellActivation.ts @@ -34,6 +34,7 @@ export class PowershellActivationNotAvailableDiagnostic extends BaseDiagnostic { DiagnosticSeverity.Warning, DiagnosticScope.Global, resource, + undefined, 'always', ); } diff --git a/src/client/application/diagnostics/checks/pythonInterpreter.ts b/src/client/application/diagnostics/checks/pythonInterpreter.ts index a2c77e461cd5..47fbbd344822 100644 --- a/src/client/application/diagnostics/checks/pythonInterpreter.ts +++ b/src/client/application/diagnostics/checks/pythonInterpreter.ts @@ -52,6 +52,8 @@ export class InvalidPythonInterpreterDiagnostic extends BaseDiagnostic { DiagnosticSeverity.Error, DiagnosticScope.WorkspaceFolder, resource, + undefined, + 'always', ); } } diff --git a/src/test/application/diagnostics/checks/pythonInterpreter.unit.test.ts b/src/test/application/diagnostics/checks/pythonInterpreter.unit.test.ts index f2a82982e564..c132d85cba34 100644 --- a/src/test/application/diagnostics/checks/pythonInterpreter.unit.test.ts +++ b/src/test/application/diagnostics/checks/pythonInterpreter.unit.test.ts @@ -24,8 +24,11 @@ import { IDiagnosticsService, } from '../../../../client/application/diagnostics/types'; import { CommandsWithoutArgs } from '../../../../client/common/application/commands'; +import { IWorkspaceService } from '../../../../client/common/application/types'; +import { Commands } from '../../../../client/common/constants'; import { IPlatformService } from '../../../../client/common/platform/types'; import { IConfigurationService, IDisposableRegistry, IPythonSettings } from '../../../../client/common/types'; +import { Common } from '../../../../client/common/utils/localize'; import { noop } from '../../../../client/common/utils/misc'; import { IInterpreterHelper, IInterpreterService } from '../../../../client/interpreter/contracts'; import { IServiceContainer } from '../../../../client/ioc/types'; @@ -38,11 +41,17 @@ suite('Application Diagnostics - Checks Python Interpreter', () => { let settings: typemoq.IMock; let interpreterService: typemoq.IMock; let platformService: typemoq.IMock; + let workspaceService: typemoq.IMock; let helper: typemoq.IMock; const pythonPath = 'My Python Path in Settings'; let serviceContainer: typemoq.IMock; function createContainer() { serviceContainer = typemoq.Mock.ofType(); + workspaceService = typemoq.Mock.ofType(); + workspaceService.setup((w) => w.workspaceFile).returns(() => undefined); + serviceContainer + .setup((s) => s.get(typemoq.It.isValue(IWorkspaceService))) + .returns(() => workspaceService.object); messageHandler = typemoq.Mock.ofType>(); serviceContainer .setup((s) => @@ -107,17 +116,7 @@ suite('Application Diagnostics - Checks Python Interpreter', () => { diagnostic.verifyAll(); } }); - test('Can not handle non-InvalidPythonPathInterpreter diagnostics', async () => { - const diagnostic = typemoq.Mock.ofType(); - diagnostic - .setup((d) => d.code) - .returns(() => 'Something Else' as any) - .verifiable(typemoq.Times.atLeastOnce()); - const canHandle = await diagnosticService.canHandle(diagnostic.object); - expect(canHandle).to.be.equal(false, 'Invalid value'); - diagnostic.verifyAll(); - }); test('Should return diagnostics if there are no interpreters after double-checking', async () => { interpreterService .setup((i) => i.hasInterpreters()) @@ -130,7 +129,13 @@ suite('Application Diagnostics - Checks Python Interpreter', () => { const diagnostics = await diagnosticService.diagnose(undefined); expect(diagnostics).to.be.deep.equal( - [new InvalidPythonInterpreterDiagnostic(DiagnosticCodes.NoPythonInterpretersDiagnostic, undefined)], + [ + new InvalidPythonInterpreterDiagnostic( + DiagnosticCodes.NoPythonInterpretersDiagnostic, + undefined, + workspaceService.object, + ), + ], 'not the same', ); }); @@ -148,7 +153,13 @@ suite('Application Diagnostics - Checks Python Interpreter', () => { const diagnostics = await diagnosticService.diagnose(undefined); expect(diagnostics).to.be.deep.equal( - [new InvalidPythonInterpreterDiagnostic(DiagnosticCodes.InvalidPythonInterpreterDiagnostic, undefined)], + [ + new InvalidPythonInterpreterDiagnostic( + DiagnosticCodes.InvalidPythonInterpreterDiagnostic, + undefined, + workspaceService.object, + ), + ], 'not the same', ); settings.verifyAll(); @@ -175,6 +186,7 @@ suite('Application Diagnostics - Checks Python Interpreter', () => { const diagnostic = new InvalidPythonInterpreterDiagnostic( DiagnosticCodes.NoPythonInterpretersDiagnostic, undefined, + workspaceService.object, ); const cmd = ({} as any) as IDiagnosticCommand; let messagePrompt: MessageCommandPrompt | undefined; @@ -187,7 +199,10 @@ suite('Application Diagnostics - Checks Python Interpreter', () => { .setup((f) => f.createCommand( typemoq.It.isAny(), - typemoq.It.isObjectWith>({ type: 'launch' }), + typemoq.It.isObjectWith>({ + type: 'executeVSCCommand', + options: Commands.Set_Interpreter, + }), ), ) .returns(() => cmd) @@ -198,13 +213,19 @@ suite('Application Diagnostics - Checks Python Interpreter', () => { messageHandler.verifyAll(); commandFactory.verifyAll(); expect(messagePrompt).not.be.equal(undefined, 'Message prompt not set'); - expect(messagePrompt!.commandPrompts).to.be.deep.equal([{ prompt: 'Download', command: cmd }]); - expect(messagePrompt!.onClose).to.not.be.equal(undefined, 'onClose handler should be set.'); + expect(messagePrompt!.commandPrompts).to.be.deep.equal([ + { + prompt: Common.selectPythonInterpreter, + command: cmd, + }, + ]); }); + test('Handling no currently selected interpreter diagnostic should show select interpreter message', async () => { const diagnostic = new InvalidPythonInterpreterDiagnostic( DiagnosticCodes.InvalidPythonInterpreterDiagnostic, undefined, + workspaceService.object, ); const cmd = ({} as any) as IDiagnosticCommand; let messagePrompt: MessageCommandPrompt | undefined; @@ -230,15 +251,15 @@ suite('Application Diagnostics - Checks Python Interpreter', () => { messageHandler.verifyAll(); commandFactory.verifyAll(); expect(messagePrompt).not.be.equal(undefined, 'Message prompt not set'); - expect(messagePrompt!.onClose).be.equal(undefined, 'onClose handler should not be set.'); expect(messagePrompt!.commandPrompts).to.be.deep.equal([ - { prompt: 'Select Python Interpreter', command: cmd }, + { prompt: Common.selectPythonInterpreter, command: cmd }, ]); }); test('Handling no interpreters diagnostic should return select interpreter cmd', async () => { const diagnostic = new InvalidPythonInterpreterDiagnostic( DiagnosticCodes.InvalidPythonInterpreterDiagnostic, undefined, + workspaceService.object, ); const cmd = ({} as any) as IDiagnosticCommand; let messagePrompt: MessageCommandPrompt | undefined; @@ -299,6 +320,7 @@ suite('Application Diagnostics - Checks Python Interpreter', () => { const diagnostic = new InvalidPythonInterpreterDiagnostic( DiagnosticCodes.InvalidPythonInterpreterDiagnostic, undefined, + workspaceService.object, ); const cmd = ({} as any) as IDiagnosticCommand; const diagnosticServiceMock = (typemoq.Mock.ofInstance(diagnosticService) as any) as typemoq.IMock< From b19c3d735549569a837ff1f0116a9ad65dd7fbba Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Mon, 11 Jul 2022 14:01:36 -0700 Subject: [PATCH 04/10] Bring back telemetry for prompt --- .../diagnostics/checks/pythonInterpreter.ts | 24 +++++++++++- src/client/telemetry/constants.ts | 1 + src/client/telemetry/index.ts | 18 +++++++++ .../checks/pythonInterpreter.unit.test.ts | 37 +------------------ 4 files changed, 43 insertions(+), 37 deletions(-) diff --git a/src/client/application/diagnostics/checks/pythonInterpreter.ts b/src/client/application/diagnostics/checks/pythonInterpreter.ts index 47fbbd344822..517d39ac5d7b 100644 --- a/src/client/application/diagnostics/checks/pythonInterpreter.ts +++ b/src/client/application/diagnostics/checks/pythonInterpreter.ts @@ -14,10 +14,18 @@ import { BaseDiagnostic, BaseDiagnosticsService } from '../base'; import { IDiagnosticsCommandFactory } from '../commands/types'; import { DiagnosticCodes } from '../constants'; import { DiagnosticCommandPromptHandlerServiceId, MessageCommandPrompt } from '../promptHandler'; -import { DiagnosticScope, IDiagnostic, IDiagnosticCommand, IDiagnosticHandlerService } from '../types'; +import { + DiagnosticScope, + IDiagnostic, + IDiagnosticCommand, + IDiagnosticHandlerService, + IDiagnosticMessageOnCloseHandler, +} from '../types'; import { Common } from '../../../common/utils/localize'; import { Commands } from '../../../common/constants'; import { IWorkspaceService } from '../../../common/application/types'; +import { sendTelemetryEvent } from '../../../telemetry'; +import { EventName } from '../../../telemetry/constants'; const localize: nls.LocalizeFunc = nls.loadMessageBundle(); @@ -125,7 +133,8 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService { return; } const commandPrompts = this.getCommandPrompts(diagnostic); - await messageService.handle(diagnostic, { commandPrompts, message: diagnostic.message }); + const onClose = getOnCloseHandler(diagnostic); + await messageService.handle(diagnostic, { commandPrompts, message: diagnostic.message, onClose }); }), ); } @@ -143,3 +152,14 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService { ]; } } + +function getOnCloseHandler(diagnostic: IDiagnostic): IDiagnosticMessageOnCloseHandler | undefined { + if (diagnostic.code === DiagnosticCodes.NoPythonInterpretersDiagnostic) { + return (response?: string) => { + sendTelemetryEvent(EventName.PYTHON_NOT_INSTALLED_PROMPT, undefined, { + selection: response ? 'Download' : 'Ignore', + }); + }; + } + return undefined; +} diff --git a/src/client/telemetry/constants.ts b/src/client/telemetry/constants.ts index 45706d868ae5..2ab6c8a8a3ba 100644 --- a/src/client/telemetry/constants.ts +++ b/src/client/telemetry/constants.ts @@ -26,6 +26,7 @@ export enum EventName { PYTHON_INTERPRETER_ACTIVATION_FOR_TERMINAL = 'PYTHON_INTERPRETER_ACTIVATION_FOR_TERMINAL', TERMINAL_SHELL_IDENTIFICATION = 'TERMINAL_SHELL_IDENTIFICATION', PYTHON_INTERPRETER_ACTIVATE_ENVIRONMENT_PROMPT = 'PYTHON_INTERPRETER_ACTIVATE_ENVIRONMENT_PROMPT', + PYTHON_NOT_INSTALLED_PROMPT = 'PYTHON_NOT_INSTALLED_PROMPT', CONDA_INHERIT_ENV_PROMPT = 'CONDA_INHERIT_ENV_PROMPT', ENVFILE_VARIABLE_SUBSTITUTION = 'ENVFILE_VARIABLE_SUBSTITUTION', ENVFILE_WORKSPACE = 'ENVFILE_WORKSPACE', diff --git a/src/client/telemetry/index.ts b/src/client/telemetry/index.ts index 017f41a72ed3..7f056894f79e 100644 --- a/src/client/telemetry/index.ts +++ b/src/client/telemetry/index.ts @@ -1319,6 +1319,24 @@ export interface IEventNamePropertyMapping { */ selection: 'Yes' | 'No' | 'Ignore' | undefined; }; + /** + * Telemetry event sent with details when the user clicks a button in the "Python is not installed" prompt. + * * `Prompt message` :- 'Python is not installed. Please download and install Python before using the extension.' + */ + /* __GDPR__ + "python_not_installed_prompt" : { + "selection" : { "classification": "SystemMetaData", "purpose": "FeatureInsight", "owner": "karrtikr" } + } + */ + [EventName.PYTHON_NOT_INSTALLED_PROMPT]: { + /** + * `Download` When the 'Download' option is clicked + * `Ignore` When the prompt is dismissed + * + * @type {('Download' | 'Ignore' | undefined)} + */ + selection: 'Download' | 'Ignore' | undefined; + }; /** * Telemetry event sent when the experiments service is initialized for the first time. */ diff --git a/src/test/application/diagnostics/checks/pythonInterpreter.unit.test.ts b/src/test/application/diagnostics/checks/pythonInterpreter.unit.test.ts index c132d85cba34..d331110a88e3 100644 --- a/src/test/application/diagnostics/checks/pythonInterpreter.unit.test.ts +++ b/src/test/application/diagnostics/checks/pythonInterpreter.unit.test.ts @@ -182,7 +182,7 @@ suite('Application Diagnostics - Checks Python Interpreter', () => { settings.verifyAll(); interpreterService.verifyAll(); }); - test('Handling no interpreters diagnostic should return download link', async () => { + test('Handling no interpreters diagnostic should return select interpreter cmd', async () => { const diagnostic = new InvalidPythonInterpreterDiagnostic( DiagnosticCodes.NoPythonInterpretersDiagnostic, undefined, @@ -219,6 +219,7 @@ suite('Application Diagnostics - Checks Python Interpreter', () => { command: cmd, }, ]); + expect(messagePrompt!.onClose).to.not.be.equal(undefined, 'onClose handler should be set.'); }); test('Handling no currently selected interpreter diagnostic should show select interpreter message', async () => { @@ -254,41 +255,7 @@ suite('Application Diagnostics - Checks Python Interpreter', () => { expect(messagePrompt!.commandPrompts).to.be.deep.equal([ { prompt: Common.selectPythonInterpreter, command: cmd }, ]); - }); - test('Handling no interpreters diagnostic should return select interpreter cmd', async () => { - const diagnostic = new InvalidPythonInterpreterDiagnostic( - DiagnosticCodes.InvalidPythonInterpreterDiagnostic, - undefined, - workspaceService.object, - ); - const cmd = ({} as any) as IDiagnosticCommand; - let messagePrompt: MessageCommandPrompt | undefined; - messageHandler - .setup((i) => i.handle(typemoq.It.isValue(diagnostic), typemoq.It.isAny())) - .callback((_d, p: MessageCommandPrompt) => (messagePrompt = p)) - .returns(() => Promise.resolve()) - .verifiable(typemoq.Times.once()); - commandFactory - .setup((f) => - f.createCommand( - typemoq.It.isAny(), - typemoq.It.isObjectWith>({ - type: 'executeVSCCommand', - }), - ), - ) - .returns(() => cmd) - .verifiable(typemoq.Times.once()); - - await diagnosticService.handle([diagnostic]); - - messageHandler.verifyAll(); - commandFactory.verifyAll(); - expect(messagePrompt).not.be.equal(undefined, 'Message prompt not set'); expect(messagePrompt!.onClose).be.equal(undefined, 'onClose handler should not be set.'); - expect(messagePrompt!.commandPrompts).to.be.deep.equal([ - { prompt: 'Select Python Interpreter', command: cmd }, - ]); }); test('Handling an empty diagnostic should not show a message nor return a command', async () => { const diagnostics: IDiagnostic[] = []; From 3dcfb949fe4d2d01b72544d956b6a7c21e6d76b3 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Mon, 11 Jul 2022 14:03:21 -0700 Subject: [PATCH 05/10] Remove comment --- .../diagnostics/checks/invalidLaunchJsonDebugger.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/client/application/diagnostics/checks/invalidLaunchJsonDebugger.ts b/src/client/application/diagnostics/checks/invalidLaunchJsonDebugger.ts index 7a955271f3a3..440ff16856d3 100644 --- a/src/client/application/diagnostics/checks/invalidLaunchJsonDebugger.ts +++ b/src/client/application/diagnostics/checks/invalidLaunchJsonDebugger.ts @@ -130,9 +130,6 @@ export class InvalidLaunchJsonDebuggerService extends BaseDiagnosticsService { } private async handleDiagnostic(diagnostic: IDiagnostic): Promise { - // if (!this.canHandle(diagnostic)) { - // return; - // } if (!diagnostic.shouldShowPrompt) { await this.fixLaunchJson(diagnostic.code); return; From 896120d0f92b761fd80b8b02f7e9e8b1538468a5 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Mon, 11 Jul 2022 14:14:30 -0700 Subject: [PATCH 06/10] Add scope to diagnostic --- .../diagnostics/checks/pythonInterpreter.ts | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/client/application/diagnostics/checks/pythonInterpreter.ts b/src/client/application/diagnostics/checks/pythonInterpreter.ts index 517d39ac5d7b..760054e4eb60 100644 --- a/src/client/application/diagnostics/checks/pythonInterpreter.ts +++ b/src/client/application/diagnostics/checks/pythonInterpreter.ts @@ -45,6 +45,7 @@ export class InvalidPythonInterpreterDiagnostic extends BaseDiagnostic { code: DiagnosticCodes.NoPythonInterpretersDiagnostic | DiagnosticCodes.InvalidPythonInterpreterDiagnostic, resource: Resource, workspaceService: IWorkspaceService, + scope = DiagnosticScope.WorkspaceFolder, ) { let formatArg = ''; if (workspaceService.workspaceFile) { @@ -54,15 +55,7 @@ export class InvalidPythonInterpreterDiagnostic extends BaseDiagnostic { formatArg = ` for ${path.basename(folder.uri.fsPath)}`; } } - super( - code, - messages[code].format(formatArg), - DiagnosticSeverity.Error, - DiagnosticScope.WorkspaceFolder, - resource, - undefined, - 'always', - ); + super(code, messages[code].format(formatArg), DiagnosticSeverity.Error, scope, resource, undefined, 'always'); } } @@ -93,6 +86,7 @@ export class InvalidPythonInterpreterService extends BaseDiagnosticsService { DiagnosticCodes.NoPythonInterpretersDiagnostic, resource, workspaceService, + DiagnosticScope.Global, ), ]; } From 96ff03442d68213dc1859e90ec07205a24dc1052 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Mon, 11 Jul 2022 14:18:39 -0700 Subject: [PATCH 07/10] Only display folder name for more than one workspace folder scenario --- .../application/diagnostics/checks/pythonInterpreter.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/client/application/diagnostics/checks/pythonInterpreter.ts b/src/client/application/diagnostics/checks/pythonInterpreter.ts index 760054e4eb60..279f63834232 100644 --- a/src/client/application/diagnostics/checks/pythonInterpreter.ts +++ b/src/client/application/diagnostics/checks/pythonInterpreter.ts @@ -48,11 +48,15 @@ export class InvalidPythonInterpreterDiagnostic extends BaseDiagnostic { scope = DiagnosticScope.WorkspaceFolder, ) { let formatArg = ''; - if (workspaceService.workspaceFile) { + if ( + workspaceService.workspaceFile && + workspaceService.workspaceFolders && + workspaceService.workspaceFolders?.length > 1 + ) { // Specify folder name in case of multiroot scenarios const folder = workspaceService.getWorkspaceFolder(resource); if (folder) { - formatArg = ` for ${path.basename(folder.uri.fsPath)}`; + formatArg = ` ${localize('Common.for', 'for')} ${path.basename(folder.uri.fsPath)}`; } } super(code, messages[code].format(formatArg), DiagnosticSeverity.Error, scope, resource, undefined, 'always'); From bc2155a18d018ab8ecb3643cf9704c79373860c8 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Mon, 11 Jul 2022 14:41:09 -0700 Subject: [PATCH 08/10] Add for workspace text --- src/client/application/diagnostics/checks/pythonInterpreter.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/application/diagnostics/checks/pythonInterpreter.ts b/src/client/application/diagnostics/checks/pythonInterpreter.ts index 279f63834232..132d86c96f97 100644 --- a/src/client/application/diagnostics/checks/pythonInterpreter.ts +++ b/src/client/application/diagnostics/checks/pythonInterpreter.ts @@ -56,7 +56,7 @@ export class InvalidPythonInterpreterDiagnostic extends BaseDiagnostic { // Specify folder name in case of multiroot scenarios const folder = workspaceService.getWorkspaceFolder(resource); if (folder) { - formatArg = ` ${localize('Common.for', 'for')} ${path.basename(folder.uri.fsPath)}`; + formatArg = ` ${localize('Common.forWorkspace', 'for workspace')} ${path.basename(folder.uri.fsPath)}`; } } super(code, messages[code].format(formatArg), DiagnosticSeverity.Error, scope, resource, undefined, 'always'); From 7f1eb4a9088d06eb1e7d3a14b90f78fad02a0e60 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Mon, 11 Jul 2022 14:48:54 -0700 Subject: [PATCH 09/10] Fix test --- .../diagnostics/checks/pythonInterpreter.unit.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/test/application/diagnostics/checks/pythonInterpreter.unit.test.ts b/src/test/application/diagnostics/checks/pythonInterpreter.unit.test.ts index d331110a88e3..bbf3f95eaa78 100644 --- a/src/test/application/diagnostics/checks/pythonInterpreter.unit.test.ts +++ b/src/test/application/diagnostics/checks/pythonInterpreter.unit.test.ts @@ -18,6 +18,7 @@ import { MessageCommandPrompt, } from '../../../../client/application/diagnostics/promptHandler'; import { + DiagnosticScope, IDiagnostic, IDiagnosticCommand, IDiagnosticHandlerService, @@ -134,6 +135,7 @@ suite('Application Diagnostics - Checks Python Interpreter', () => { DiagnosticCodes.NoPythonInterpretersDiagnostic, undefined, workspaceService.object, + DiagnosticScope.Global, ), ], 'not the same', From 8449f03d42f50dd8c0a9dbecd0537980ffc06b79 Mon Sep 17 00:00:00 2001 From: Kartik Raj Date: Mon, 11 Jul 2022 15:00:43 -0700 Subject: [PATCH 10/10] Update src/client/application/diagnostics/checks/pythonInterpreter.ts Co-authored-by: Karthik Nadig --- src/client/application/diagnostics/checks/pythonInterpreter.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/client/application/diagnostics/checks/pythonInterpreter.ts b/src/client/application/diagnostics/checks/pythonInterpreter.ts index 132d86c96f97..8befefe47bb5 100644 --- a/src/client/application/diagnostics/checks/pythonInterpreter.ts +++ b/src/client/application/diagnostics/checks/pythonInterpreter.ts @@ -32,7 +32,7 @@ const localize: nls.LocalizeFunc = nls.loadMessageBundle(); const messages = { [DiagnosticCodes.NoPythonInterpretersDiagnostic]: localize( 'DiagnosticCodes.NoPythonInterpretersDiagnostic', - 'No Python interpreter is selected. You need to select a Python interpreter to enable features such as IntelliSense, linting, and debugging.', + 'No Python interpreter is selected. Please select a Python interpreter to enable features such as IntelliSense, linting, and debugging.', ), [DiagnosticCodes.InvalidPythonInterpreterDiagnostic]: localize( 'DiagnosticCodes.NoCurrentlySelectedPythonInterpreterDiagnostic',