Remove unnecessary nullables #9766
Conversation
| @@ -182,6 +182,7 @@ export class JupyterExecutionBase implements IJupyterExecution { | |||
| } | |||
| throw lastTryError; | |||
| } | |||
| throw new Error('Max number of attempts reached'); | |||
There was a problem hiding this comment.
This function was the root cause, we have an explicit return undefined and an implicit at the end of the while loop.
That causes return value to have undefined and bubbles all the way the to the top.
This is an unlikely scenario (cancellation is handled higher up, hence we don't need to swallow cancellations and disposing classes here)
| if (!this.connection) { | ||
| return undefined; | ||
| throw new Error('Not connected'); |
There was a problem hiding this comment.
THis is impossible, when the class is contructed, the first things that called is the connect method that initializes the property this.connection. Ideally we shoudl remove such code that initializes the class and then calls methods.
I've found this in a number of places and replaced them with factories, after that we dont end up with nullables (removing the nullabels makes the code simpler, e.g. in this case, IJupyterConnection can never be null in the any code path, but due to the way the class is written we don't know hence end up returning undefined and have to handle this case everywhere its used)
There was a problem hiding this comment.
This class is now instantiated via a factory, hence connection is never nullable and the method getConnectionInfo is now a property
| @inject(IDisposableRegistry) private readonly disposables: IDisposableRegistry | ||
| @inject(IDisposableRegistry) private readonly disposables: IDisposableRegistry, | ||
| public connection: IJupyterConnection, | ||
| private sessionManager: JupyterSessionManager |
There was a problem hiding this comment.
The connection property is accessed using getConnection and we have some code that waits for this to be initialized.
Basically as soon as this class is contructed, the first method invoked is connect and its impossible for connection to be undefined, however due to the way the class was constructed it can be undefined in types & we have code all over the place that treats it as such.
Refactored this class to be created via a factory so that it can never be nullable, as a result the code paths are much simpler now.
No more nullables.
| } | ||
| } catch (e) { | ||
| traceError(`Error during shutdown: `, e); | ||
| } | ||
| } | ||
|
|
||
| private waitForConnect(): Promise<IJupyterConnection | undefined> { | ||
| return this.connectPromise.promise; |
There was a problem hiding this comment.
Funky code now a thing of the past due to nullables being removed.
| @@ -34,27 +34,35 @@ import { JupyterNotebook } from '../jupyterNotebook'; | |||
| import { noop } from '../../../../platform/common/utils/misc'; | |||
| import { Cancellation } from '../../../../platform/common/cancellation'; | |||
| import { getDisplayPath } from '../../../../platform/common/platform/fs-paths'; | |||
| import { INotebookServer, IJupyterSessionManagerFactory } from '../../types'; | |||
| import { INotebookServer } from '../../types'; | |||
| import { Uri } from 'vscode'; | |||
| /* eslint-disable @typescript-eslint/no-explicit-any */ | |||
|
|
|||
| @injectable() | |||
There was a problem hiding this comment.
You should probably remove this now then. Otherwise I think the tests will fail saying it's not in the container.
Codecov Report
@@ Coverage Diff @@
## main #9766 +/- ##
====================================
Coverage 63% 63%
====================================
Files 203 203
Lines 9816 9816
Branches 1556 1556
====================================
+ Hits 6215 6221 +6
+ Misses 3104 3096 -8
- Partials 497 499 +2
|
| @@ -152,7 +151,7 @@ export class JupyterExecutionBase implements IJupyterExecution { | |||
| } else if (connection) { | |||
| // If this is occurring during shutdown, don't worry about it. | |||
There was a problem hiding this comment.
update comment? (Or just remove)
Part of #9167
Basically we have a lot of unnecessary nullables, thereby simplifying the code
This PR finally fixes this comment