-
Notifications
You must be signed in to change notification settings - Fork 338
Fix error links in web to work #10471
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
Conversation
@@ -105,27 +118,27 @@ export class ErrorRendererCommunicationHandler implements IExtensionSyncActivati | |||
const uri = Uri.parse(cellUri); | |||
|
|||
// Show the matching notebook if there is one | |||
let editor = this.notebooks.notebookEditors.find((n) => arePathsSame(n.notebook.uri.fsPath, uri.fsPath)); |
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.
Paths here don't work on web, so switched to just finding the cell instead.
@@ -48,9 +48,6 @@ export class NotebookTracebackFormatter implements ITracebackFormatter { | |||
|
|||
const inputMatch = /^Input.*?\[.*32mIn\s+\[(\d+).*?0;36m(.*?)\n.*/.exec(traceFrame); | |||
if (inputMatch && inputMatch.length > 1) { | |||
const executionCount = parseInt(inputMatch[1]); | |||
// Don't assume the current cell is where the error is. |
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.
This wasn't working if the execution count is the same in a previous cell. Not sure how in a notebook formatter the error would come from a different cell? Removed this as I don't think we actually need it.
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.
This might have been leftover code from when the formatter was shared between the interactive window and a notebook.
// This should eventually give focus to the code cell | ||
await waitForCondition( | ||
async () => { | ||
return window.activeTextEditor?.document === codeCell.document; |
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.
This is how we check for 'focus' in a cell.
} | ||
|
||
// Public for testing | ||
public async onDidReceiveMessage(e: { |
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.
You mentioned for testing, but honestly I like this better just as a general thing. Lambdas always look funny to me when they get really long.
Fixes #10287
Root cause - comm message handler wasn't registered in web.
Also fixed some issues with it jumping to the wrong cell if other cells had the same execution count.
Also added a test.