Handle reloading of REPL Window #24148
Conversation
anthonykim1
added
area-repl
feature-request
anthonykim1
added
bug
src/client/repl/nativeRepl.ts
Outdated
| wsMementoUri = wsMemento ? Uri.parse(wsMemento) : undefined; | ||
|
|
||
| if (!wsMementoUri || getTabNameForUri(wsMementoUri) !== 'Python REPL') { | ||
| await this.cleanRepl(); |
There was a problem hiding this comment.
You are in here because this.notebookDocument is undefined. So why are you clearing it again? also, it does not seem like it gets set or used here (in this if block) so why is this being checked.
There was a problem hiding this comment.
this is for case: #24148 (comment)
This case, memento URI will be referencing old native REPL, whereas in reality the untitled notebook have already taken over the URI of the old native REPL. In this case, to prevent native REPL creeping into the untitled jupyter notebook, we have to explicitly check for if the URI belongs to Python REPL, and properly clean up everything so that new native REPL can be created with different URI.
There was a problem hiding this comment.
my question here was about this.notebookDocument. Its state does not seem to change within this if, so why are we clearing it?
There was a problem hiding this comment.
Ahhh I read this wrong, I understand this now. Yeah it wont change inside the if block, so no need at all:
c3648f5
| if (notebookDocument) { | ||
| if (mementoValue) { | ||
| if (!notebookDocument) { | ||
| notebookDocument = await workspace.openNotebookDocument(mementoValue as Uri); |
There was a problem hiding this comment.
Avoid depending directly on workspace, window, commands. if you want to use our Unit Test framework effectively. One of the things you do with Unit testing is control the boundary. These APIs are the boundary for the extension, and using them directly like this can break that boundary too often. Also, this is a broad namespace, makes it hard to mock.
There was a problem hiding this comment.
10000% agree. I created #24426
I think I should go through major refactoring for native REPL code soon, and replace all these workspace, window APIs
| @@ -20,22 +21,39 @@ import { PVSC_EXTENSION_ID } from '../common/constants'; | |||
| export async function openInteractiveREPL( | |||
| notebookController: NotebookController, | |||
| notebookDocument: NotebookDocument | undefined, | |||
| ): Promise<NotebookEditor> { | |||
| mementoValue: Uri | undefined, | |||
There was a problem hiding this comment.
It feels like this should not be passed to this function. The function already takes a notebookDocument. You can make notebookDocument arg flexible NotebookDocument | Uri | undefined . If notebook document is instance of Uri, use openNotebookDocument, else use the notebookDocument.
Resolves: #24021