Simplify args used to start/connect to jupyter #9888
Conversation
| @@ -234,12 +234,12 @@ export class JupyterExecutionBase implements IJupyterExecution { | |||
| if (options.localJupyter) { | |||
| // If that works, then attempt to start the server | |||
| traceInfo(`Launching server`); | |||
| const useDefaultConfig = !options || options.skipUsingDefaultConfig ? false : true; | |||
There was a problem hiding this comment.
This value in the options property was totally useless, as we can see the next line we access the settings.
hence why not access the settings to determine whther we're to skip using default config here itself instead of passing a property around.
This simplifies the type a lot
| // Expand the working directory. Create a dummy launching file in the root path (so we expand correctly) | ||
| const workingDirectory = expandWorkingDir( | ||
| options.workingDir, |
There was a problem hiding this comment.
This value in the options property was totally useless, as we can see the next line we access the workspace object.
hence why not compute the workspace directory here instead of passing properties around.
This simplifies the type a lot, fewer properties.
This simplifies the type a lot
| @@ -24,8 +24,7 @@ export class JupyterNotebookProvider implements IJupyterNotebookProvider { | |||
| ) {} | |||
|
|
|||
| public async connect(options: ConnectNotebookProviderOptions): Promise<IJupyterConnection> { | |||
| const server = await this.serverProvider.getOrCreateServer(options); | |||
| const connection = await server.connection; | |||
| const { connection } = await this.serverProvider.getOrCreateServer(options); | |||
There was a problem hiding this comment.
Unnecessar await
| @@ -81,8 +79,6 @@ export type INotebookServerOptions = | |||
| | { | |||
| uri: string; | |||
| resource: Resource; | |||
| skipUsingDefaultConfig?: boolean; | |||
| workingDir?: string; | |||
| ui: IDisplayOptions; | |||
There was a problem hiding this comment.
Now the options to start a notebook server is much smaller.
| ): Promise<IJupyterConnection | undefined> { | ||
| return this.notebookStarter?.start( | ||
| ): Promise<IJupyterConnection> { | ||
| if (!this.notebookStarter) { |
There was a problem hiding this comment.
This isn't possible in desktop. hence throw an error.
There was a problem hiding this comment.
Doesn't need to be localized, its basically a bug in the code that would cause the entire extensoin to go kaboom.
Wish there was a better way to handle such types, these nullables really do complicate the code.
There was a problem hiding this comment.
Hopefully this will get fixed with the refactoring I'm working on.
Codecov Report
@@ Coverage Diff @@
## main #9888 +/- ##
====================================
Coverage 63% 63%
====================================
Files 203 203
Lines 9825 9825
Branches 1564 1564
====================================
+ Hits 6239 6245 +6
+ Misses 3086 3078 -8
- Partials 500 502 +2
|
Part of #8933