这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@akshita31
Copy link
Contributor

@akshita31 akshita31 commented Aug 23, 2019

Addresses #395.

@akshita31 akshita31 changed the title Load extensions using #r nuget directive WIP: Load extensions using #r nuget directive Aug 23, 2019
{
public static class ExtensionLoader
{
public static async Task LoadFromAssembly(Assembly assembly, IKernel kernel)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be internal.

Copy link
Contributor Author

@akshita31 akshita31 Aug 24, 2019

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 ?

@akshita31 akshita31 force-pushed the load_extension_using_#r_directive branch 2 times, most recently from 6df3f1f to 5a48b9e Compare August 24, 2019 00:17
@akshita31 akshita31 force-pushed the load_extension_using_#r_directive branch 2 times, most recently from 4ef00fd to 7e5a1fe Compare August 29, 2019 01:19
{
public class ExtensionLoaded : KernelEventBase
{
public ExtensionLoaded(string extensionPath)
Copy link
Contributor

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 =>
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

@akshita31 akshita31 force-pushed the load_extension_using_#r_directive branch from 296aee2 to 6640d36 Compare August 30, 2019 21:20
@akshita31 akshita31 changed the title WIP: Load extensions using #r nuget directive Load extensions using #r nuget directive Aug 30, 2019
@akshita31 akshita31 force-pushed the load_extension_using_#r_directive branch 2 times, most recently from 734bb07 to fabc267 Compare August 30, 2019 23:55
{
public interface IExtensibleKernel
{
public Task LoadExtensionsInDirectory(IDirectoryAccessor directory, KernelInvocationContext invocationContext);
Copy link
Contributor

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?

Copy link
Contributor Author

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"));
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@akshita31 akshita31 force-pushed the load_extension_using_#r_directive branch from 3bdfd88 to 981b614 Compare September 3, 2019 19:43
@akshita31 akshita31 force-pushed the load_extension_using_#r_directive branch from 981b614 to 0c4c9ee Compare September 3, 2019 20:01
@akshita31 akshita31 force-pushed the load_extension_using_#r_directive branch from 1cd71fc to f14b681 Compare September 4, 2019 23:13
@akshita31 akshita31 force-pushed the load_extension_using_#r_directive branch from f14b681 to c9eed93 Compare September 4, 2019 23:14
@akshita31 akshita31 merged commit 3a98248 into dotnet:master Sep 5, 2019
@akshita31 akshita31 deleted the load_extension_using_#r_directive branch September 5, 2019 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants