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

Make value kernel extensible #2939

Merged
merged 2 commits into from
Apr 25, 2023

Conversation

colombod
Copy link
Member

No description provided.

{
mimeType ??= (options.MimeType ?? PlainTextFormatter.MimeType);

var shouldDisplayValue = options.MimeType is { } displayMimeType;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: The displayMimeType variable is unused and can be dropped.

}

return Task.CompletedTask;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the method be made sync or is this async because we think we may need it down the line?

@@ -143,7 +143,7 @@ Task IKernelCommandHandler<SendValue>.HandleAsync(SendValue command, KernelInvoc
context.Fail(command,
message: "The --from-file option cannot be used in combination with a content submission.");
}

Copy link
Contributor

Choose a reason for hiding this comment

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

minor: Remove blank line.

string value,
string mimeType,
bool shouldDisplayValue,
KernelInvocationContext context)

Copy link
Contributor

Choose a reason for hiding this comment

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

minor: Consider removing blank line...

Copy link
Contributor

@shyamnamboodiripad shyamnamboodiripad left a comment

Choose a reason for hiding this comment

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

Change looks good - however, I am not sure why this is needed. Could you please clarify? Thanks!

@colombod colombod merged commit e1620e3 into dotnet:main Apr 25, 2023
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.

2 participants