-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
variable provider for native repl #24094
Conversation
You may need to rebase after this other PR is merged for the Smoke test issue: 2b4c2bf |
indexedChildrenCount: number; | ||
} | ||
|
||
interface NotebookVariableProvider { |
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.
@amunger Does the vscode engine need to be explicitly pinned?
We currently have
"engines": {
"vscode": "^1.94.0-20240913"
},
also saw commit on vscode-distro so I think we are good.
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.
just has to be beyond that distro commit, and that version is, so it should work
function wrapScriptInFunction(scriptLines: string[]): string { | ||
const indented = scriptLines.map((line) => ` ${line}`).join('\n'); | ||
// put everything into a function scope and then delete that scope | ||
// TODO: run in a background thread |
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.
Would running the script in the background give performance benefit?
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.
having a large number of variables could conceivably block execution as they are collected and putting this operation on a background thread would prevent that. the speed of safeRepr might make that unnecessary though.
fix #24066