enable display update #388
Conversation
| } | ||
|
|
||
| public FormattedValue FormattedValue { get; } | ||
| public string Id { get; } |
There was a problem hiding this comment.
For consistency, DisplayId would be better.
There was a problem hiding this comment.
Although I think this identifies a "value", not a "display", so here an in UpdateDisplayValue maybe we should rename this to ValueId?
|
|
||
| namespace Microsoft.DotNet.Interactive.Commands | ||
| { | ||
| public class UpdateDisplayValue : KernelCommandBase |
There was a problem hiding this comment.
Maybe UpdateDisplayedValue?
| bool isLastValue = false, | ||
| IReadOnlyCollection<FormattedValue> formattedValues = null) : base(command) | ||
| IReadOnlyCollection<FormattedValue> formattedValues = null, | ||
| string id= null, |
|
|
||
| public string Id { get; } | ||
|
|
||
| public bool IsUpdatedValue { get; } |
There was a problem hiding this comment.
It seems like this should implicitly be true if an id is set. Would it not be a coding error if this is set to true while id is set to null?
There was a problem hiding this comment.
is false if id is not set, but can be false even if Id is set
| { | ||
| public interface IDisplayHandle | ||
| { | ||
| void Update(object value); |
There was a problem hiding this comment.
Maybe UpdateDisplayedValue?
| { | ||
| public class UpdateDisplayedValue : KernelCommandBase | ||
| { | ||
| public UpdateDisplayedValue(FormattedValue formattedValue, string valueId = null) |
There was a problem hiding this comment.
valueId is required, right? I don't think it should default to null.
| return Task.CompletedTask; | ||
| }; | ||
|
|
||
| displayedValue.Handler = invocationContext => |
There was a problem hiding this comment.
Doesn't this just over-write the Handler set in the line above this?
| .And | ||
| .ContainSingle(v => | ||
| v.MimeType == "text/html" && | ||
| v.Value.ToString().Contains("<b>world</b>")); |
There was a problem hiding this comment.
This assertion doesn't check that they're also in the correct order.
8e7d137 to
0799c88
Compare
| { | ||
| if (isUpdatedValue && valueId == null) | ||
| { | ||
| throw new ArgumentException("isUpdatedValue cannot be true with a null valueId", nameof(valueId)); |
There was a problem hiding this comment.
Please use nameof for the referenced parameter names.
ffe50aa to
679798f
Compare
display api returns a displayHandle that can be used to update the output in teh frontend