forked from DonJayamanne/pythonVSCode
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add a "Jupyter not installed" notification helper #16321
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Kim-Adeline Miguel (kimadeline)
merged 12 commits into
microsoft:16102-jupyter-dependency
from
kimadeline:16102-jupyter-notification-helper
May 31, 2021
Merged
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
a4b527b
Add telemetry info
c7dce83
Use enum for the telemetry
253266c
Add prompt as a standalone function
41a5c4e
Merge branch 'main' into 16102-jupyter-notification-helper
3ec463e
Remove "Install" from the prompt
5febf45
Make it a class
e2b40f7
Register singleton
20fbb7d
Rename file to a long but descriptive name
f3faa30
Unit tests
9f9dbcf
Merge branch '16102-jupyter-dependency' into 16102-jupyter-notificati…
96e22db
Add to package.nls.json
4fc76bd
Use sinon for tests
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
58 changes: 58 additions & 0 deletions
58
src/client/jupyter/jupyterNotInstalledNotificationHelper.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| import { injectable, inject } from 'inversify'; | ||
| import { IApplicationShell, IJupyterExtensionDependencyManager } from '../common/application/types'; | ||
| import { IPersistentStateFactory } from '../common/types'; | ||
| import { Common, Jupyter } from '../common/utils/localize'; | ||
| import { sendTelemetryEvent } from '../telemetry'; | ||
| import { EventName } from '../telemetry/constants'; | ||
| import { IJupyterNotInstalledNotificationHelper, JupyterNotInstalledOrigin } from './types'; | ||
|
|
||
| export const jupyterExtensionNotInstalledKey = 'jupyterExtensionNotInstalledKey'; | ||
|
|
||
| @injectable() | ||
| export class JupyterNotInstalledNotificationHelper implements IJupyterNotInstalledNotificationHelper { | ||
| constructor( | ||
| @inject(IApplicationShell) private appShell: IApplicationShell, | ||
| @inject(IPersistentStateFactory) private persistentState: IPersistentStateFactory, | ||
| @inject(IJupyterExtensionDependencyManager) private depsManager: IJupyterExtensionDependencyManager, | ||
| ) {} | ||
|
|
||
| public shouldShowJupypterExtensionNotInstalledPrompt(): boolean { | ||
| const doNotShowAgain = this.persistentState.createGlobalPersistentState(jupyterExtensionNotInstalledKey, false); | ||
|
|
||
| if (doNotShowAgain.value) { | ||
| return false; | ||
| } | ||
|
|
||
| const isInstalled = this.depsManager.isJupyterExtensionInstalled; | ||
|
|
||
| return !isInstalled; | ||
| } | ||
|
|
||
| public async jupyterNotInstalledPrompt(entrypoint: JupyterNotInstalledOrigin): Promise<void> { | ||
| sendTelemetryEvent(EventName.JUPYTER_NOT_INSTALLED_NOTIFICATION_DISPLAYED, undefined, { entrypoint }); | ||
|
|
||
| const prompts = [Common.doNotShowAgain()]; | ||
| const telemetrySelections: ['Do not show again'] = ['Do not show again']; | ||
|
|
||
| const selection = await this.appShell.showInformationMessage( | ||
| Jupyter.jupyterExtensionNotInstalled(), | ||
| ...prompts, | ||
| ); | ||
|
|
||
| sendTelemetryEvent(EventName.JUPYTER_NOT_INSTALLED_NOTIFICATION_ACTION, undefined, { | ||
| selection: selection ? telemetrySelections[prompts.indexOf(selection)] : undefined, | ||
| }); | ||
|
|
||
| if (!selection) { | ||
| return; | ||
| } | ||
|
|
||
| // Never show this prompt again | ||
| await this.persistentState | ||
| .createGlobalPersistentState(jupyterExtensionNotInstalledKey, false) | ||
| .updateValue(true); | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,3 +45,17 @@ enum ColumnType { | |
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| type IRowsResponse = any[]; | ||
|
|
||
| // Note: While #16102 is being worked on, this enum will be updated as we add ways to display this notification. | ||
| export enum JupyterNotInstalledOrigin { | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While #16102 is being worked on, this enum will be updated as we add ways to display this notification. |
||
| StartPageCreateBlankNotebook = 'startpage_create_blank_notebook', | ||
| StartPageCreateJupyterNotebook = 'startpage_create_jupyter_notebook', | ||
| StartPageCreateSampleNotebook = 'startpage_sample_notebook', | ||
| StartPageUseInteractiveWindow = 'startpage_use_interactive_window', | ||
| } | ||
|
|
||
| export const IJupyterNotInstalledNotificationHelper = Symbol('IJupyterNotInstalledNotificationHelper'); | ||
| export interface IJupyterNotInstalledNotificationHelper { | ||
| shouldShowJupypterExtensionNotInstalledPrompt(): boolean; | ||
| jupyterNotInstalledPrompt(entrypoint: JupyterNotInstalledOrigin): Promise<void>; | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
151 changes: 151 additions & 0 deletions
151
src/test/jupyter/jupyterNotInstalledNotificationHelper.unit.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,151 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| import * as assert from 'assert'; | ||
| import * as sinon from 'sinon'; | ||
| import { IApplicationShell, IJupyterExtensionDependencyManager } from '../../client/common/application/types'; | ||
| import { IPersistentStateFactory } from '../../client/common/types'; | ||
| import { Jupyter, Common } from '../../client/common/utils/localize'; | ||
| import { | ||
| jupyterExtensionNotInstalledKey, | ||
| JupyterNotInstalledNotificationHelper, | ||
| } from '../../client/jupyter/jupyterNotInstalledNotificationHelper'; | ||
| import { JupyterNotInstalledOrigin } from '../../client/jupyter/types'; | ||
|
|
||
| suite('Jupyter not installed notification helper', () => { | ||
| teardown(() => { | ||
| sinon.restore(); | ||
| }); | ||
|
|
||
| test('Notification check should return false if the Jupyter extension is installed', () => { | ||
| const createGlobalPersistentStateStub = sinon | ||
| .stub() | ||
| .withArgs(jupyterExtensionNotInstalledKey, sinon.match.bool) | ||
| .returns({ value: undefined }); | ||
|
|
||
| // Need to define 'isJupyterExtensionInstalled' for it to be stubbed. | ||
| const jupyterExtDependencyManager = { | ||
| isJupyterExtensionInstalled: false, | ||
| } as IJupyterExtensionDependencyManager; | ||
| const isJupyterExtensionInstalledStub = sinon.stub().returns(true); | ||
| sinon.stub(jupyterExtDependencyManager, 'isJupyterExtensionInstalled').get(isJupyterExtensionInstalledStub); | ||
|
|
||
| const notificationHelper = new JupyterNotInstalledNotificationHelper( | ||
| {} as IApplicationShell, | ||
| ({ createGlobalPersistentState: createGlobalPersistentStateStub } as unknown) as IPersistentStateFactory, | ||
| jupyterExtDependencyManager, | ||
| ); | ||
|
|
||
| const result = notificationHelper.shouldShowJupypterExtensionNotInstalledPrompt(); | ||
|
|
||
| assert.strictEqual(result, false); | ||
| sinon.assert.calledOnce(createGlobalPersistentStateStub); | ||
| sinon.assert.calledWith(createGlobalPersistentStateStub, jupyterExtensionNotInstalledKey, sinon.match.bool); | ||
| sinon.assert.calledOnce(isJupyterExtensionInstalledStub); | ||
| }); | ||
|
|
||
| test('Notification check should return false if the doNotShowAgain persistent value is set', () => { | ||
| const createGlobalPersistentStateStub = sinon | ||
| .stub() | ||
| .withArgs(jupyterExtensionNotInstalledKey, sinon.match.bool) | ||
| .returns({ value: true }); | ||
|
|
||
| const jupyterExtDependencyManager = { | ||
| isJupyterExtensionInstalled: false, | ||
| } as IJupyterExtensionDependencyManager; | ||
| const isJupyterExtensionInstalledStub = sinon.stub().returns(false); | ||
| sinon.stub(jupyterExtDependencyManager, 'isJupyterExtensionInstalled').get(isJupyterExtensionInstalledStub); | ||
|
|
||
| const notificationHelper = new JupyterNotInstalledNotificationHelper( | ||
| {} as IApplicationShell, | ||
| ({ createGlobalPersistentState: createGlobalPersistentStateStub } as unknown) as IPersistentStateFactory, | ||
| jupyterExtDependencyManager, | ||
| ); | ||
|
|
||
| const result = notificationHelper.shouldShowJupypterExtensionNotInstalledPrompt(); | ||
|
|
||
| assert.strictEqual(result, false); | ||
| sinon.assert.calledOnce(createGlobalPersistentStateStub); | ||
| sinon.assert.calledWith(createGlobalPersistentStateStub, jupyterExtensionNotInstalledKey, sinon.match.bool); | ||
| sinon.assert.notCalled(isJupyterExtensionInstalledStub); | ||
| }); | ||
|
|
||
| test('Notification check should return true if the doNotShowAgain persistent value is not set and the Jupyter extension is not installed', () => { | ||
| const createGlobalPersistentStateStub = sinon | ||
| .stub() | ||
| .withArgs(jupyterExtensionNotInstalledKey, sinon.match.bool) | ||
| .returns({ value: undefined }); | ||
|
|
||
| const jupyterExtDependencyManager = { | ||
| isJupyterExtensionInstalled: false, | ||
| } as IJupyterExtensionDependencyManager; | ||
| const isJupyterExtensionInstalledStub = sinon.stub().returns(false); | ||
| sinon.stub(jupyterExtDependencyManager, 'isJupyterExtensionInstalled').get(isJupyterExtensionInstalledStub); | ||
|
|
||
| const notificationHelper = new JupyterNotInstalledNotificationHelper( | ||
| {} as IApplicationShell, | ||
| ({ createGlobalPersistentState: createGlobalPersistentStateStub } as unknown) as IPersistentStateFactory, | ||
| (jupyterExtDependencyManager as unknown) as IJupyterExtensionDependencyManager, | ||
| ); | ||
|
|
||
| const result = notificationHelper.shouldShowJupypterExtensionNotInstalledPrompt(); | ||
|
|
||
| assert.strictEqual(result, true); | ||
| sinon.assert.calledOnce(createGlobalPersistentStateStub); | ||
| sinon.assert.calledWith(createGlobalPersistentStateStub, jupyterExtensionNotInstalledKey, sinon.match.bool); | ||
| sinon.assert.calledOnce(isJupyterExtensionInstalledStub); | ||
| }); | ||
|
|
||
| test('Selecting "Do not show again" should set the doNotShowAgain persistent value', async () => { | ||
| const updateValueStub = sinon.stub(); | ||
| const createGlobalPersistentStateStub = sinon | ||
| .stub() | ||
| .withArgs(jupyterExtensionNotInstalledKey, sinon.match.bool) | ||
| .returns({ updateValue: updateValueStub }); | ||
|
|
||
| const showInformationMessageStub = sinon.stub().returns(Promise.resolve(Common.doNotShowAgain)); | ||
|
|
||
| const notificationHelper = new JupyterNotInstalledNotificationHelper( | ||
| ({ showInformationMessage: showInformationMessageStub } as unknown) as IApplicationShell, | ||
| ({ createGlobalPersistentState: createGlobalPersistentStateStub } as unknown) as IPersistentStateFactory, | ||
| {} as IJupyterExtensionDependencyManager, | ||
| ); | ||
| await notificationHelper.jupyterNotInstalledPrompt(JupyterNotInstalledOrigin.StartPageCreateBlankNotebook); | ||
|
|
||
| sinon.assert.calledOnce(createGlobalPersistentStateStub); | ||
| sinon.assert.calledOnce(showInformationMessageStub); | ||
| sinon.assert.calledWith( | ||
| showInformationMessageStub, | ||
| Jupyter.jupyterExtensionNotInstalled(), | ||
| Common.doNotShowAgain(), | ||
| ); | ||
| sinon.assert.calledOnce(updateValueStub); | ||
| sinon.assert.calledWith(updateValueStub, true); | ||
| }); | ||
|
|
||
| test('Selecting "Do not show again" should make the prompt check return false', async () => { | ||
| const persistentState: { value: boolean | undefined; updateValue: (v: boolean) => void } = { | ||
| value: undefined, | ||
| updateValue(v: boolean) { | ||
| this.value = v; | ||
| }, | ||
| }; | ||
| const createGlobalPersistentStateStub = sinon | ||
| .stub() | ||
| .withArgs(jupyterExtensionNotInstalledKey, sinon.match.bool) | ||
| .returns(persistentState); | ||
|
|
||
| const showInformationMessageStub = sinon.stub().returns(Promise.resolve(Common.doNotShowAgain)); | ||
|
|
||
| const notificationHelper = new JupyterNotInstalledNotificationHelper( | ||
| ({ showInformationMessage: showInformationMessageStub } as unknown) as IApplicationShell, | ||
| ({ createGlobalPersistentState: createGlobalPersistentStateStub } as unknown) as IPersistentStateFactory, | ||
| {} as IJupyterExtensionDependencyManager, | ||
| ); | ||
| await notificationHelper.jupyterNotInstalledPrompt(JupyterNotInstalledOrigin.StartPageCreateBlankNotebook); | ||
|
|
||
| const result = notificationHelper.shouldShowJupypterExtensionNotInstalledPrompt(); | ||
|
|
||
| assert.strictEqual(result, false); | ||
| }); | ||
| }); |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like something which can be a diagnostic, like
PylanceDefaultDiagnosticyou recently implemented.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not something we are going to run on start, but that will be displayed in response to users taking specific actions at different points in the extension. What would be the advantage of using diagnostics in that case compared to showing a notification?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see, sorry I missed that.
Diagnostics already have the do not show again functionality so you don't have to create a separate key, and it's consistent with a similar class we have:
vscode-python/src/client/application/diagnostics/checks/invalidPythonPathInDebugger.ts
Line 48 in e3e773a
which triggers in response to debugging. But eh, it's not much of an advantage, so not changing it is fine.