Remote 'kernelCommandListener' as it really listens to notebooks #10369
Conversation
| Commands.NotebookEditorInterruptKernel, | ||
| (context?: { notebookEditor: { notebookUri: Uri } } | Uri) => { | ||
| if (context && 'notebookEditor' in context) { | ||
| void this.interruptKernel(context?.notebookEditor?.notebookUri); |
There was a problem hiding this comment.
Please can you swallow the promise rejections using .ignoreErrors or .catch(noop)
as follows:
this.interruptKernel(context?.notebookEditor?.notebookUri).catch(noop);
// or
this.interruptKernel(context?.notebookEditor?.notebookUri).ignoreErrors();Using void doesn't really handle the errors, hence if something fails we get errors logged by node.js about unhandled errors and this causes a few issues when debugging and the like.
Will mention at standup today.
There was a problem hiding this comment.
Sure. This was already there, but I can fix it.
There was a problem hiding this comment.
We should add a linter rule.
There was a problem hiding this comment.
Or I can just search for 'void this' everywhere and fix it.
There was a problem hiding this comment.
I've got a PR for that, will mention in standup.
|
@DonJayamanne did you have any more feedback? |
|
Not sure why i didn't approve, sorry |
For #10152
Another step in the removal of the 'notebook' references in kernels. The KernelCommandListener class was really listening to commands on notebooks so I just moved it.