Add setKernelSpecAndLanguageInfo to ipynb ext #132298
Conversation
There was a problem hiding this comment.
Thanks! Looks like the right overall direction but I think I'd prefer a single function instead of two
Can you also please try building VS Code with this fix to just confirm it handles your use case: https://github.com/microsoft/vscode/wiki/How-to-Contribute
That should cut down on back and forths since I don't have a good understand of the specifics of what you need on the Julia side
extensions/ipynb/src/ipynbMain.ts
Outdated
| @@ -41,6 +41,26 @@ export function activate(context: vscode.ExtensionContext) { | |||
| }); | |||
| return vscode.workspace.applyEdit(edit); | |||
| }, | |||
| setKernelSpecAndLanguageInfo: async (resource: vscode.Uri, kernelspec: unknown, language_info: unknown): Promise<boolean> => { | |||
There was a problem hiding this comment.
Can languageInfo be an optional argument to setKernelSpec instead of its own function?
There was a problem hiding this comment.
Done. I'm not entirely sure, though, whether the way I'm construction the custom dictionary now will work... In particular, just not entirely sure whether ...(language_info ? { language_info: language_info } : {}) does what I think it does.
There was a problem hiding this comment.
@mjbvz @davidanthoff Why not change the method name completely and pass the entire metadata?
I.e. as follows:
setNotebookMetaata(resoruce: vscode.Uri, metadata: Partial<nbformat.INotbeookMetadata>): Promise<boolean> {
...
}After all, the requirement is to provide an API to set the metadata. and if we end up with more items in the notebook metadata the new method will be future proof.
Today there are at least 3 properties in the metadata (kernelSpec, language_info and orig_nbformat).
I propose changing the method to providing a partial of the metadata.
This will also allow others to add custom metadata into the notebook (after all the ipynb spec allows for that).
/**
* The default metadata for the notebook.
*/
interface INotebookMetadata extends JSONObject {
kernelspec?: IKernelspecMetadata;
language_info?: ILanguageInfoMetadata;
orig_nbformat: number;
}There was a problem hiding this comment.
Yes, that sounds like a good idea to me. Although the most important objective from our end is to get something into 1.60.1, I think :) @DonJayamanne do you want to take over this PR? You're probably in the best position to do the right thing here.
I don't have the bandwidth to do that before sometime mid next week, sorry.
Maybe @DonJayamanne can help out? He wrote the original code in the Julia extension that preps all the metadata, all we need is an API that allows us to finish the TODO that @DonJayamanne left there. |
|
Once done, I'll ensure we use this same API in the Jupyter extension as well. |
|
Updated the PR, will test and get back. |
There was a problem hiding this comment.
@DonJayamanne this looks great! In particular the option to add arbitrary additional metadata, I think that could be very useful down the road.
I left a couple of points where maybe we should not make some fields part of the API here? But I don't think this is a biggy, they are all optional, so probably doesn't matter that much.
There was a problem hiding this comment.
Approving my PR, as its blocked by me.
@mjbvz I think we should remove the other API, not sure whether we do that now or later.
Done |
Following @mjbvz' suggestion over at https://github.com/julia-vscode/julia-vscode/pull/2424/files#r702128257.
I'm not setup to build VS Code, so this is a "blind" PR that I haven't tested.
I added a new API function here, under the assumption that the one that just shipped needs to stay stable as it is part of the public API now.