From f65b327ce219a13fc8daa4f129359a521d5db0e5 Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Wed, 2 Jun 2021 09:05:04 -0700 Subject: [PATCH 1/3] rename to showJupyterNotInstalledPrompt --- src/client/jupyter/jupyterNotInstalledNotificationHelper.ts | 2 +- src/client/jupyter/types.ts | 2 +- .../jupyterNotInstalledNotificationHelper.unit.test.ts | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/client/jupyter/jupyterNotInstalledNotificationHelper.ts b/src/client/jupyter/jupyterNotInstalledNotificationHelper.ts index 328fa23ca955..c3c7ec578702 100644 --- a/src/client/jupyter/jupyterNotInstalledNotificationHelper.ts +++ b/src/client/jupyter/jupyterNotInstalledNotificationHelper.ts @@ -31,7 +31,7 @@ export class JupyterNotInstalledNotificationHelper implements IJupyterNotInstall return !isInstalled; } - public async jupyterNotInstalledPrompt(entrypoint: JupyterNotInstalledOrigin): Promise { + public async showJupyterNotInstalledPrompt(entrypoint: JupyterNotInstalledOrigin): Promise { sendTelemetryEvent(EventName.JUPYTER_NOT_INSTALLED_NOTIFICATION_DISPLAYED, undefined, { entrypoint }); const prompts = [Common.doNotShowAgain()]; diff --git a/src/client/jupyter/types.ts b/src/client/jupyter/types.ts index 14d0c868adbf..dec66cdcb729 100644 --- a/src/client/jupyter/types.ts +++ b/src/client/jupyter/types.ts @@ -57,5 +57,5 @@ export enum JupyterNotInstalledOrigin { export const IJupyterNotInstalledNotificationHelper = Symbol('IJupyterNotInstalledNotificationHelper'); export interface IJupyterNotInstalledNotificationHelper { shouldShowJupypterExtensionNotInstalledPrompt(): boolean; - jupyterNotInstalledPrompt(entrypoint: JupyterNotInstalledOrigin): Promise; + showJupyterNotInstalledPrompt(entrypoint: JupyterNotInstalledOrigin): Promise; } diff --git a/src/test/jupyter/jupyterNotInstalledNotificationHelper.unit.test.ts b/src/test/jupyter/jupyterNotInstalledNotificationHelper.unit.test.ts index f82b9cb0c8a6..6323e30aa09c 100644 --- a/src/test/jupyter/jupyterNotInstalledNotificationHelper.unit.test.ts +++ b/src/test/jupyter/jupyterNotInstalledNotificationHelper.unit.test.ts @@ -110,7 +110,7 @@ suite('Jupyter not installed notification helper', () => { ({ createGlobalPersistentState: createGlobalPersistentStateStub } as unknown) as IPersistentStateFactory, {} as IJupyterExtensionDependencyManager, ); - await notificationHelper.jupyterNotInstalledPrompt(JupyterNotInstalledOrigin.StartPageCreateBlankNotebook); + await notificationHelper.showJupyterNotInstalledPrompt(JupyterNotInstalledOrigin.StartPageCreateBlankNotebook); sinon.assert.calledOnce(createGlobalPersistentStateStub); sinon.assert.calledOnce(showInformationMessageStub); @@ -142,7 +142,7 @@ suite('Jupyter not installed notification helper', () => { ({ createGlobalPersistentState: createGlobalPersistentStateStub } as unknown) as IPersistentStateFactory, {} as IJupyterExtensionDependencyManager, ); - await notificationHelper.jupyterNotInstalledPrompt(JupyterNotInstalledOrigin.StartPageCreateBlankNotebook); + await notificationHelper.showJupyterNotInstalledPrompt(JupyterNotInstalledOrigin.StartPageCreateBlankNotebook); const result = notificationHelper.shouldShowJupypterExtensionNotInstalledPrompt(); From da736ae759c0b951dad202045c2db82082303b74 Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Wed, 2 Jun 2021 10:00:19 -0700 Subject: [PATCH 2/3] Replace existing prompt with new prompt --- .../common/application/commandManager.ts | 16 +++++++++++----- src/client/common/application/types.ts | 1 - src/client/common/utils/localize.ts | 5 ----- .../jupyterExtensionDependencyManager.ts | 18 ++---------------- src/client/jupyter/types.ts | 1 + 5 files changed, 14 insertions(+), 27 deletions(-) diff --git a/src/client/common/application/commandManager.ts b/src/client/common/application/commandManager.ts index 498eec10f59a..5fa9b99f297a 100644 --- a/src/client/common/application/commandManager.ts +++ b/src/client/common/application/commandManager.ts @@ -3,14 +3,15 @@ import { inject, injectable } from 'inversify'; import { commands, Disposable, TextEditor, TextEditorEdit } from 'vscode'; +import { IJupyterNotInstalledNotificationHelper, JupyterNotInstalledOrigin } from '../../jupyter/types'; import { ICommandNameArgumentTypeMapping } from './commands'; -import { ICommandManager, IJupyterExtensionDependencyManager } from './types'; +import { ICommandManager } from './types'; @injectable() export class CommandManager implements ICommandManager { constructor( - @inject(IJupyterExtensionDependencyManager) - private jupyterExtensionDependencyManager: IJupyterExtensionDependencyManager, + @inject(IJupyterNotInstalledNotificationHelper) + private jupyterNotInstalledNotificationHelper: IJupyterNotInstalledNotificationHelper, ) {} /** @@ -73,8 +74,13 @@ export class CommandManager implements ICommandManager { E extends keyof ICommandNameArgumentTypeMapping, U extends ICommandNameArgumentTypeMapping[E] >(command: E, ...rest: U): Thenable { - if (command.includes('jupyter') && !this.jupyterExtensionDependencyManager.isJupyterExtensionInstalled) { - return this.jupyterExtensionDependencyManager.installJupyterExtension(this); + if ( + command.includes('jupyter') && + !this.jupyterNotInstalledNotificationHelper.shouldShowJupypterExtensionNotInstalledPrompt() + ) { + return this.jupyterNotInstalledNotificationHelper + .showJupyterNotInstalledPrompt(JupyterNotInstalledOrigin.JupyterCommand) + .then(() => undefined); } else { return commands.executeCommand(command, ...rest); } diff --git a/src/client/common/application/types.ts b/src/client/common/application/types.ts index 01df5613bd90..5f3e27014ef7 100644 --- a/src/client/common/application/types.ts +++ b/src/client/common/application/types.ts @@ -488,7 +488,6 @@ export interface ICommandManager { export const IJupyterExtensionDependencyManager = Symbol('IJupyterExtensionDependencyManager'); export interface IJupyterExtensionDependencyManager { readonly isJupyterExtensionInstalled: boolean; - installJupyterExtension(commandManager: ICommandManager): Promise; } export const IDocumentManager = Symbol('IDocumentManager'); diff --git a/src/client/common/utils/localize.ts b/src/client/common/utils/localize.ts index 80fd801ba783..6614f8017284 100644 --- a/src/client/common/utils/localize.ts +++ b/src/client/common/utils/localize.ts @@ -137,11 +137,6 @@ export namespace Pylance { } export namespace Jupyter { - export const jupyterExtensionRequired = localize( - 'Jupyter.extensionRequired', - 'The Jupyter extension is required to perform that task. Click Yes to open the Jupyter extension installation page.', - ); - export const jupyterExtensionNotInstalled = localize( 'Jupyter.extensionNotInstalled', "This feature is available in the Jupyter extension, which isn't currently installed.", diff --git a/src/client/jupyter/jupyterExtensionDependencyManager.ts b/src/client/jupyter/jupyterExtensionDependencyManager.ts index 0db458eac051..defd5ea38241 100644 --- a/src/client/jupyter/jupyterExtensionDependencyManager.ts +++ b/src/client/jupyter/jupyterExtensionDependencyManager.ts @@ -1,27 +1,13 @@ import { inject, injectable } from 'inversify'; -import { IApplicationShell, ICommandManager, IJupyterExtensionDependencyManager } from '../common/application/types'; +import { IJupyterExtensionDependencyManager } from '../common/application/types'; import { JUPYTER_EXTENSION_ID } from '../common/constants'; import { IExtensions } from '../common/types'; -import { Common, Jupyter } from '../common/utils/localize'; @injectable() export class JupyterExtensionDependencyManager implements IJupyterExtensionDependencyManager { - constructor( - @inject(IExtensions) private extensions: IExtensions, - @inject(IApplicationShell) private appShell: IApplicationShell, - ) {} + constructor(@inject(IExtensions) private extensions: IExtensions) {} public get isJupyterExtensionInstalled(): boolean { return this.extensions.getExtension(JUPYTER_EXTENSION_ID) !== undefined; } - - public async installJupyterExtension(commandManager: ICommandManager): Promise { - const yes = Common.bannerLabelYes(); - const no = Common.bannerLabelNo(); - const answer = await this.appShell.showErrorMessage(Jupyter.jupyterExtensionRequired(), yes, no); - if (answer === yes) { - commandManager.executeCommand('extension.open', JUPYTER_EXTENSION_ID); - } - return undefined; - } } diff --git a/src/client/jupyter/types.ts b/src/client/jupyter/types.ts index dec66cdcb729..731a3593cc09 100644 --- a/src/client/jupyter/types.ts +++ b/src/client/jupyter/types.ts @@ -52,6 +52,7 @@ export enum JupyterNotInstalledOrigin { StartPageCreateJupyterNotebook = 'startpage_create_jupyter_notebook', StartPageCreateSampleNotebook = 'startpage_sample_notebook', StartPageUseInteractiveWindow = 'startpage_use_interactive_window', + JupyterCommand = 'jupyter_command', } export const IJupyterNotInstalledNotificationHelper = Symbol('IJupyterNotInstalledNotificationHelper'); From 8e244e85da5621b00d07bad734d7dfe189701bc4 Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Wed, 2 Jun 2021 11:43:47 -0700 Subject: [PATCH 3/3] Remove Jupyter check from command manager --- .../common/application/commandManager.ts | 19 +++---------------- src/client/jupyter/types.ts | 1 - 2 files changed, 3 insertions(+), 17 deletions(-) diff --git a/src/client/common/application/commandManager.ts b/src/client/common/application/commandManager.ts index 5fa9b99f297a..b0ddb4d60198 100644 --- a/src/client/common/application/commandManager.ts +++ b/src/client/common/application/commandManager.ts @@ -1,18 +1,14 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. -import { inject, injectable } from 'inversify'; +import { injectable } from 'inversify'; import { commands, Disposable, TextEditor, TextEditorEdit } from 'vscode'; -import { IJupyterNotInstalledNotificationHelper, JupyterNotInstalledOrigin } from '../../jupyter/types'; import { ICommandNameArgumentTypeMapping } from './commands'; import { ICommandManager } from './types'; @injectable() export class CommandManager implements ICommandManager { - constructor( - @inject(IJupyterNotInstalledNotificationHelper) - private jupyterNotInstalledNotificationHelper: IJupyterNotInstalledNotificationHelper, - ) {} + constructor() {} /** * Registers a command that can be invoked via a keyboard shortcut, @@ -74,16 +70,7 @@ export class CommandManager implements ICommandManager { E extends keyof ICommandNameArgumentTypeMapping, U extends ICommandNameArgumentTypeMapping[E] >(command: E, ...rest: U): Thenable { - if ( - command.includes('jupyter') && - !this.jupyterNotInstalledNotificationHelper.shouldShowJupypterExtensionNotInstalledPrompt() - ) { - return this.jupyterNotInstalledNotificationHelper - .showJupyterNotInstalledPrompt(JupyterNotInstalledOrigin.JupyterCommand) - .then(() => undefined); - } else { - return commands.executeCommand(command, ...rest); - } + return commands.executeCommand(command, ...rest); } /** diff --git a/src/client/jupyter/types.ts b/src/client/jupyter/types.ts index 731a3593cc09..dec66cdcb729 100644 --- a/src/client/jupyter/types.ts +++ b/src/client/jupyter/types.ts @@ -52,7 +52,6 @@ export enum JupyterNotInstalledOrigin { StartPageCreateJupyterNotebook = 'startpage_create_jupyter_notebook', StartPageCreateSampleNotebook = 'startpage_sample_notebook', StartPageUseInteractiveWindow = 'startpage_use_interactive_window', - JupyterCommand = 'jupyter_command', } export const IJupyterNotInstalledNotificationHelper = Symbol('IJupyterNotInstalledNotificationHelper');