-
Notifications
You must be signed in to change notification settings - Fork 563
Load extensions using #r nuget directive #396
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
Load extensions using #r nuget directive #396
Conversation
| { | ||
| public static class ExtensionLoader | ||
| { | ||
| public static async Task LoadFromAssembly(Assembly assembly, IKernel kernel) |
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.
This can be internal.
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.
If we make this internal, how will it be accessible from the tests ?
6df3f1f to
5a48b9e
Compare
WorkspaceServer.Tests/NugetPackageKernelExtensionFinderTests.cs
Outdated
Show resolved
Hide resolved
4ef00fd to
7e5a1fe
Compare
Microsoft.DotNet.Interactive/Commands/LoadExtensionInNuGetPackage.cs
Outdated
Show resolved
Hide resolved
Microsoft.DotNet.Interactive/Commands/LoadExtensionInNuGetPackage.cs
Outdated
Show resolved
Hide resolved
| { | ||
| public class ExtensionLoaded : KernelEventBase | ||
| { | ||
| public ExtensionLoaded(string extensionPath) |
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.
If we think about display feedback, I wonder if something a little richer than a string would be useful. What other use cases might there be for this event? What can subscribers do with a string?
| context, | ||
| next), | ||
|
|
||
| LoadExtensionInNuGetPackage loadCSharpExtension => |
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.
Is this is C#-specific? If so, it should go into the CSharpKernel. This is worth discussing with @TIHan.
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.
This is something we need to have a discussion with @KevinRansom about as he has been working on #r nuget for F#. It was mentioned that we could and probably should share so that there isn't a difference in behavior between two implementations of resolving nuget packages for the interactive experience.
I imagine that F# by itself could have its own way of resolving nuget packages, but through an API allow a custom handler.
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.
I don't think we need a different way to resolve a NuGet package but might need custom handling once it's resolved, if only for different extension paths within the package.
296aee2 to
6640d36
Compare
734bb07 to
fabc267
Compare
| { | ||
| public interface IExtensibleKernel | ||
| { | ||
| public Task LoadExtensionsInDirectory(IDirectoryAccessor directory, KernelInvocationContext invocationContext); |
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.
Will there always be an invocation context? What about extensions that we want to load on startup?
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.
We need to "Publish" an event saying that the extension has been loaded and we need the context for that. If we would like to load extensions at startup, how would we "Publish" that event if we dont have the KernelInvocationContext ?
I think once we have that scenario, it would not be a hard change to make this method maybe return a list of extensions that it loaded(so that it can be called at startup) and then the caller can do whatever necessary. Thoughts ?
| var directory = Create.EmptyWorkspace().Directory; | ||
|
|
||
| const string nugetPackageName = "myNugetPackage"; | ||
| var nugetPackageDirectory = new FileSystemDirectoryAccessor(directory.Subdirectory($"{nugetPackageName}/2.0.0")); |
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.
Can we test this using InMemoryDirectoryAccessor?
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.
This is an end to end test which tests that given a nuget package dll, is it able to load the extension from that, and since "LoadAssembly" is being called, it needs the artifacts to actually exist on disk.
3bdfd88 to
981b614
Compare
981b614 to
0c4c9ee
Compare
1cd71fc to
f14b681
Compare
f14b681 to
c9eed93
Compare
Addresses #395.