Skip to content
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

Fix error links in web to work #10471

Merged
merged 3 commits into from
Jun 16, 2022
Merged

Fix error links in web to work #10471

merged 3 commits into from
Jun 16, 2022

Conversation

rchiodo
Copy link
Contributor

@rchiodo rchiodo commented Jun 16, 2022

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.

@rchiodo rchiodo requested a review from a team as a code owner June 16, 2022 17:00
@@ -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));
Copy link
Contributor Author

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.
Copy link
Contributor Author

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.

Copy link
Contributor Author

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;
Copy link
Contributor Author

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: {
Copy link
Member

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.

@rchiodo rchiodo merged commit 726574a into main Jun 16, 2022
@rchiodo rchiodo deleted the rchiodo/links_in_web branch June 16, 2022 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error links in notebooks cannot be clicked in web extension
2 participants