tracking std out prints #343
Conversation
| new ValueProduced("value three", kernelCommand, new[] { new FormattedValue(null, "value three"), })); | ||
| new ValueProduced("value one", kernelCommand, false, new[] { new FormattedValue(null, "value one"), }), | ||
| new ValueProduced("value two", kernelCommand, false, new[] { new FormattedValue(null, "value two"), }), | ||
| new ValueProduced("value three", kernelCommand, false, new[] { new FormattedValue(null, "value three"), })); |
There was a problem hiding this comment.
myme types are done later on when formatting on raw value is done
There was a problem hiding this comment.
I think FormattedValue without a mime type is confusing and probably bug prone. I think this test shows that because of the redundancy -- the formatted values ARE the raw values.
There was a problem hiding this comment.
null value is generated by the Formatter.MimeTypeFor method
There was a problem hiding this comment.
That's the right behavior. FormattedValue.MimeType is intentionally read-only because the expectation was that the mime type must be known at the time a FormattedValue is created. It was an oversight that the constructor didn't guard against this.
|
|
||
| public override void Write(char value) | ||
| { | ||
| TrackSize(() => base.Write(value)); |
There was a problem hiding this comment.
You're going to reallocate this delegate every time you write, consider caching it.
| private async Task TrackSizeAsync(Func<Task> action) | ||
| { | ||
| WriteOccurred = true; | ||
| if (_trakingSize) |
There was a problem hiding this comment.
I feel like this is trying to handle some concurrency scenario but there doesn't seem to be any synchronization?
There was a problem hiding this comment.
is handling re-entrancy, writeLine get decombosed in base class in write(value) and then WriteLine()
| data: valueProduced?.FormattedValues | ||
| ?.ToDictionary(k => k.MimeType ?? "text/plain", v => v.Value)) | ||
| : new DisplayData( | ||
| transient: transient, | ||
| data: valueProduced?.FormattedValues | ||
| ?.ToDictionary(k => k.MimeType ?? "text/plain", v => v.Value)); |
There was a problem hiding this comment.
We should no longer need a fallback here.
| @@ -358,8 +358,7 @@ private static void TryRegisterDefault(string typeName, Action<object, TextWrite | |||
|
|
|||
| public static string MimeTypeFor(Type type) | |||
| { | |||
| _mimeTypesByType.TryGetValue(type, out var mimeType); | |||
| return mimeType; | |||
| return _mimeTypesByType.TryGetValue(type, out var mimeType) ? mimeType : "text/plain"; | |||
There was a problem hiding this comment.
Maybe extract a static property Default so we don't keep repeating this string?
| namespace Microsoft.DotNet.Interactive | ||
| { | ||
| public class FormattedValue | ||
| { | ||
| public FormattedValue(string mimeType, object value) | ||
| { | ||
| MimeType = mimeType; | ||
| MimeType = mimeType ?? throw new ArgumentNullException(nameof(mimeType)); |
There was a problem hiding this comment.
This should also check for empty and whitespace, which are also not valid mime types.
doing Console.Write and Console.WriteLine in kernels produces values