Skip to content

Conversation

@kimadeline
Copy link

@kimadeline kimadeline commented May 25, 2021

For #16102, will be used in later PRs wherever needed.

Tests have been updated to use sinon.

@kimadeline kimadeline added the no-changelog No news entry required label May 25, 2021
@kimadeline kimadeline marked this pull request as ready for review May 26, 2021 14:28
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 {
Copy link
Author

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 {

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.

Copy link
Author

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?

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:

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.

Copy link

@karrtikr karrtikr left a 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 {

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:

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.

Copy link

@paulacamargo25 paulacamargo25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job, LGTM!

@kimadeline kimadeline merged commit d294a98 into microsoft:16102-jupyter-dependency May 31, 2021
@kimadeline kimadeline deleted the 16102-jupyter-notification-helper branch May 31, 2021 19:56
karrtikr pushed a commit that referenced this pull request Jun 8, 2021
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog No news entry required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants