-
Notifications
You must be signed in to change notification settings - Fork 121
PositroNB extension API integration #9843
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
…ShowNotebookDocument
a115b59
to
9fa2fec
Compare
E2E Tests 🚀 |
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.
Code looks great.
The adapter raises lots of questions about how we can/should update the base implementation to make this translation easier (as long as it doesn't make the main implementation more complicated.)
// Get all Positron notebook instances | ||
const instances = new Map<string, IPositronNotebookInstance>(); | ||
for (const instance of this._positronNotebookService.listInstances()) { | ||
if (instance.textModel) { |
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.
Wont this always be true because textModel is an observable? Should it be instance.textModel.get()
?
super.dispose(); | ||
} | ||
} | ||
|
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.
/** | |
* Extracts the Positron notebook instance from a generic editor pane, if it contains one. | |
* | |
* The workbench editor service deals with generic `IEditorPane` objects that could | |
* represent any type of editor (text, notebook, diff, etc.). When integrating Positron notebooks | |
* with the extension API, we need to identify which editor panes contain Positron notebooks. | |
* | |
* @returns The notebook instance if the pane is a Positron notebook editor, undefined otherwise | |
*/ |
I found myself needing to look at this to get context so a docstring may help.
This PR integrates Positron notebook editors with the extension API, addresses #9440. This is a more focused alternative to #9709.
Release Notes
New Features
Bug Fixes
QA Notes
Nothing should break. I'll follow up with another PR that adds cell debugging to Positron notebooks which will make it possible to QA this functionality.
@:notebooks