-
Notifications
You must be signed in to change notification settings - Fork 290
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
Experiment: manual jupyter server connection through uri server provider api #11945
Conversation
@@ -43,6 +43,9 @@ export class JupyterUriProviderWrapper implements IJupyterUriProvider { | |||
public get displayName(): string | undefined { | |||
return this.provider.displayName; | |||
} | |||
public get detail(): string | undefined { |
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.
id
is not sufficient for the new quick pick, thus we introduced displayname and detail. We will need to revisit the updated API soon.
@@ -72,6 +72,57 @@ export type SelectJupyterUriCommandSource = | |||
| 'errorHandler' | |||
| 'prompt'; | |||
|
|||
export async function validateSelectJupyterURI( |
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.
I thought about implementing everything completely in standalone
but that would be quite some effort so for now I'm still using helper functions from the core.
@@ -77,10 +74,6 @@ interface ContributedKernelFinderQuickPickItem extends QuickPickItem { | |||
kernelFinderInfo: IContributedKernelFinder; | |||
} | |||
|
|||
interface LocalJupyterServerQuickPickItem extends QuickPickItem { |
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.
Removed this special case in the picker and moved towards serverUriProvider api.
@@ -85,4 +86,13 @@ export function registerTypes(context: IExtensionContext, serviceManager: IServi | |||
|
|||
// Dev Tools | |||
registerDevToolTypes(context, serviceManager, isDevMode); | |||
|
|||
const configuration = serviceManager.get<IConfigurationService>(IConfigurationService); | |||
if (configuration.getSettings().kernelPickerType === 'Insiders') { |
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.
We are checking this in multiple places, wish we can have a more elegant approach ;(
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.
I wanted to create a single class with a single readonly property,
Codecov Report
@@ Coverage Diff @@
## main #11945 +/- ##
=======================================
- Coverage 62% 62% -1%
=======================================
Files 492 493 +1
Lines 34795 34883 +88
Branches 5640 5646 +6
=======================================
- Hits 21877 21655 -222
- Misses 10845 11168 +323
+ Partials 2073 2060 -13
|
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.
Lets talk about accessing data from credential store in activate event tomorrow, else everything else seems good.
@DonJayamanne comments resolved, please help take another look. |
readonly detail: string = DataScience.UserJupyterServerUrlProviderDetail(); | ||
private _onDidChangeHandles = new EventEmitter<void>(); | ||
onDidChangeHandles: Event<void> = this._onDidChangeHandles.event; | ||
private readonly _globalDisposables: IDisposable[] = []; |
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.
These aren't disposed,
you'd want to pull in IDisposableRegistry
into the ctor
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.
Just need to dispose the disposables, else all good
Implemented the manual jupyter server support through uri server provider api in
src/standalone
. Previously we had this support as a builtin concept (in other words, a special case), but now it is treated like other uri server provider (e.g. AZML). The implementation is not perfect yet but can be used to drive the improvement forThis feature is behind the same setting as experimental kernel picker, so won't affect Insider users yet.
package-lock.json
has been regenerated by runningnpm install
(if dependencies have changed).