Skip to content
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

kernelinfo carries information on kernel type #2716

Merged
merged 2 commits into from
Feb 10, 2023

Conversation

colombod
Copy link
Member

No description provided.

missing ;


fix test


use isProxy


fix test
@@ -229,10 +229,12 @@ Microsoft.DotNet.Interactive
public System.Threading.Tasks.Task<Microsoft.DotNet.Interactive.Connection.ProxyKernel> ConnectProxyKernelOnDefaultConnectorAsync(System.String localName, System.Uri remoteKernelUri, System.String[] aliases = null)
public System.Void Dispose()
public class KernelInfo
.ctor(System.String localName, System.String[] aliases = null)
.ctor(System.String localName, System.String[] aliases = null, System.Boolean isProxy = False, System.Boolean isComposite = False)
Copy link
Contributor

Choose a reason for hiding this comment

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

Having two different constructors with slightly different arguments is a little confusing and probably pointless given all the setters on this class. Maybe we should remove both constructors and just have a default constructor?

Copy link
Member Author

Choose a reason for hiding this comment

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

the other one is marked as deprecated, we should remove it remove it

.ctor(System.String localName, System.String languageName, System.String languageVersion, System.String[] aliases)
public System.String[] Aliases { get; set;}
public System.String DisplayName { get; set;}
public System.Boolean IsComposite { get;}
public System.Boolean IsProxy { get;}
Copy link
Contributor

@jonsequitur jonsequitur Feb 10, 2023

Choose a reason for hiding this comment

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

These should be mutable like the others... I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

@colombod colombod merged commit 35389b8 into dotnet:main Feb 10, 2023
@colombod colombod deleted the extend_kernel_info branch February 10, 2023 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kernelinfo should include details about the remote kernel
2 participants