updates for Microsoft.DotNet.Interactive.Documents API stabilization #3169
Conversation
| IDictionary<string, object>? metadata, | ||
| [NotNullWhen(true)] out KernelInfoCollection? kernelInfo) | ||
| [NotNullWhen(true)] out KernelInfoCollection? kernelInfos) |
There was a problem hiding this comment.
Can this be inlined into the above non-static overload (with all callers switched to calling the non-static overload)? Or can this be made private?
Please ignore the above if we have more callers for the static overload outside the current class.
There was a problem hiding this comment.
The static method has internal callers.
| @@ -65,7 +65,7 @@ public async IAsyncEnumerable<InteractiveDocument> GetImportsAsync(bool recursiv | |||
| { | |||
| EnsureImportFieldParserIsInitialized(); | |||
|
|
|||
| if (!TryGetKernelInfoFromMetadata(Metadata, out var kernelInfos)) | |||
| if (!TryGetKernelInfosFromMetadata(out var kernelInfos)) | |||
There was a problem hiding this comment.
minor nit: This call uses the public instance overload while others below use the static overload. Could we standardize to the same call?
| @@ -198,7 +198,7 @@ internal void NormalizeElementKernelNames(KernelInfoCollection kernelInfos) | |||
|
|
|||
| public string? GetDefaultKernelName() | |||
| { | |||
| if (TryGetKernelInfoFromMetadata(Metadata, out var kernelInfo)) | |||
| if (TryGetKernelInfosFromMetadata(Metadata, out var kernelInfo)) | |||
| { | |||
| return kernelInfo.DefaultKernelName; | |||
| } | |||
There was a problem hiding this comment.
Why is the public API missing the remaining heuristics (like checking for kernelspec metadata)
| @@ -264,19 +259,23 @@ internal static void MergeKernelInfos(KernelInfoCollection destination, KernelIn | |||
| destination.AddRange(source.Where(ki => added.Add(ki.Name))); | |||
| } | |||
|
|
|||
| internal static bool TryGetKernelInfoFromMetadata( | |||
| public bool TryGetKernelInfosFromMetadata( | |||
There was a problem hiding this comment.
This seems like a new public API. I don't remember adding this during the sync - was this exposed intentionally?
There was a problem hiding this comment.
We discussed this during the sync, that there's currently no way to get the kernel info using the public API, and that it's very useful to have.
No description provided.