remove IKernelConnector #3108
remove IKernelConnector #3108
Conversation
jonsequitur
force-pushed
the
remove-IKernelConnector
branch
from
1d4c365
to
8cf64d1
Compare
| var localName = commandLineContext.ParseResult.GetValueForOption(KernelNameOption); | ||
| var kernel = await connector.CreateKernelAsync(localName); | ||
| var kernel1 = new SQLiteKernel($"sql-{localName}", connectionString); | ||
| var kernel = await Task.FromResult<Kernel>(kernel1); | ||
| return new[] { kernel }; |
There was a problem hiding this comment.
The await is a bit confusing. Why not remove the async keyword and say something like return Task.FromResult(new [] { new SQLiteKernel($"sql-{localName}", connectionString) });?
There was a problem hiding this comment.
That was a weird artifact of the inlining refactoring. Fixed.
| { | ||
| CreateRemoteKernelTopology(_pipeName); | ||
|
|
||
| var connector = new NamedPipeKernelConnector(_pipeName); | ||
|
|
||
| RegisterForDisposal(connector); | ||
|
|
||
| return connector; | ||
| return name => connector.CreateKernelAsync(name); |
There was a problem hiding this comment.
Can this be simplified to return connector.CreateKernelAsync?
| @@ -133,7 +133,7 @@ public async Task fast_path_commands_over_proxy_can_be_handled() | |||
| { | |||
| var connector = CreateConnector(); | |||
There was a problem hiding this comment.
minor nit: Also rename this variable from connector to createKernel to match the above test?
| /// <remarks> | ||
| /// TODO: Does it even make sense to implement <see cref="IKernelConnector"/> here considering that we have | ||
| /// concepts (such as a single root <see cref="ProxyKernel"/>, and zero or more (child) <see cref="ProxyKernel"/>s) | ||
| /// that the <see cref="IKernelConnector"/> abstraction does not understand / support? Should the '#!connect stdio' |
There was a problem hiding this comment.
Just curious - any changes in our thoughts around the #!connect stdio? Can we remove that too?
There was a problem hiding this comment.
I think we might still want this, especially as project integration comes along and we think about .NET Interactive instances for other TFMs, different dependency graphs, etc.
colombod
added
breaking change
No description provided.