-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add a "Jupyter not installed" notification helper #16321
Conversation
| 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 { |
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.
While #16102 is being worked on, this enum will be updated as we add ways to display this notification.
| @inject(IJupyterExtensionDependencyManager) private depsManager: IJupyterExtensionDependencyManager, | ||
| ) {} | ||
|
|
||
| public shouldShowJupypterExtensionNotInstalledPrompt(): boolean { |
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 PylanceDefaultDiagnostic you 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
| export class InvalidPythonPathInDebuggerService extends BaseDiagnosticsService |
which triggers in response to debugging. But eh, it's not much of an advantage, so not changing it is fine.
karrtikr
left a comment
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.
LGTM
| @inject(IJupyterExtensionDependencyManager) private depsManager: IJupyterExtensionDependencyManager, | ||
| ) {} | ||
|
|
||
| public shouldShowJupypterExtensionNotInstalledPrompt(): boolean { |
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
| export class InvalidPythonPathInDebuggerService extends BaseDiagnosticsService |
which triggers in response to debugging. But eh, it's not much of an advantage, so not changing it is fine.
paulacamargo25
left a comment
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.
Nice job, LGTM!
* Make Jupyter an optional dependency (#16267) * News entry * Move Jupyter to the optional dependencies step * Update news/1 Enhancements/16102.md Co-authored-by: Kartik Raj <[email protected]> Co-authored-by: Kartik Raj <[email protected]> * License wording update (#16278) * Wording * License wording * Add a "Jupyter not installed" notification helper (#16321) * Add telemetry info * Use enum for the telemetry * Add prompt as a standalone function * Remove "Install" from the prompt * Make it a class * Register singleton * Rename file to a long but descriptive name * Unit tests * Add to package.nls.json * Use sinon for tests * Use the same "Jupyter is not installed" message everywhere (#16372) * rename to showJupyterNotInstalledPrompt * Replace existing prompt with new prompt * Remove Jupyter check from command manager * Update the start page to use the prompt (#16417) * Update copy * Update origin key * Show prompt if jupyter not installed & should show * Add tests for this functionality only * Update news entry * Remove comments * follow-up from the merge * Add singletons for startpage functional tests * Missing one symbol * Update src/client/common/startPage/startPage.ts Co-authored-by: Don Jayamanne <[email protected]> * Add logging Co-authored-by: Don Jayamanne <[email protected]> Co-authored-by: Kartik Raj <[email protected]> Co-authored-by: Don Jayamanne <[email protected]>
For #16102, will be used in later PRs wherever needed.
Tests have been updated to use
sinon.