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.

Sign up for GitHub

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

tracking std out prints #343

Merged
merged 11 commits into from
Jul 23, 2019
Merged

tracking std out prints #343

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
51827f1
tracking std out prints
colombod Jul 23, 2019
2e5a772
transient display ids must be emitted per value
colombod Jul 23, 2019
f44251d
handle last value produce correctly
colombod Jul 23, 2019
117ee6c
remove kernel name from csharpkernel tests
colombod Jul 23, 2019
24e4b9f
mime type not null
colombod Jul 23, 2019
3d4dcdf
Merge branch 'master' into feature/console_writeline
colombod Jul 23, 2019
99be41c
rename methods
colombod Jul 23, 2019
03f2fe1
use const for DefaultMimeType and remove redundant checks
colombod Jul 23, 2019
67aba14
Merge branch 'master' into feature/console_writeline
colombod Jul 23, 2019
69fd2d7
white space
colombod Jul 23, 2019
a5be738
empty or white space string are not valid mimetypes
colombod Jul 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion Microsoft.DotNet.Interactive.Jupyter/ExecuteRequestHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,8 +159,13 @@ private static void OnValueProduced(
{
var transient = CreateTransient();
// executeResult data
var executeResultData = new ExecuteResult(
var executeResultData = valueProduced.IsLastValue
?new ExecuteResult(
openRequest.ExecutionCount,
transient: transient,
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));
Copy link
Contributor

Choose a reason for hiding this comment

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

We should no longer need a fallback here.

Expand Down
6 changes: 4 additions & 2 deletions Microsoft.DotNet.Interactive/Events/ValueProduced.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,18 @@ namespace Microsoft.DotNet.Interactive.Events
{
public class ValueProduced : KernelEventBase
{
public ValueProduced(
object value,
public ValueProduced(object value,
SubmitCode submitCode,
bool isLastValue = false,
colombod marked this conversation as resolved.
Show resolved Hide resolved
IReadOnlyCollection<FormattedValue> formattedValues = null) : base(submitCode)
{
Value = value;
IsLastValue = isLastValue;
FormattedValues = formattedValues;
}

public object Value { get; }
public bool IsLastValue { get; }

public IReadOnlyCollection<FormattedValue> FormattedValues { get; }
}
Expand Down
26 changes: 23 additions & 3 deletions WorkspaceServer.Tests/Kernel/CSharpKernelTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,9 +192,29 @@ public async Task it_produces_values_when_executing_Console_output()
KernelEvents.OfType<ValueProduced>()
.Should()
.BeEquivalentTo(
new ValueProduced("value one", kernelCommand, new[] { new FormattedValue(null, "value one"), }),
new ValueProduced("value two", kernelCommand, new[] { new FormattedValue(null, "value two"), }),
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"), }));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why no mime type?

Copy link
Member Author

Choose a reason for hiding this comment

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

myme types are done later on when formatting on raw value is done

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

null value is generated by the Formatter.MimeTypeFor method

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}

[Fact]
public async Task it_produces_a_final_value_if_the_code_expression_evaluates()
{
var kernel = CreateKernel();

var kernelCommand = new SubmitCode(@"
Console.Write(""value one"");
Console.Write(""value two"");
Console.Write(""value three"");
5", "csharp");
colombod marked this conversation as resolved.
Show resolved Hide resolved
await kernel.SendAsync(kernelCommand);

KernelEvents.OfType<ValueProduced>()
.Should()
.HaveCount(4)
.And
.ContainSingle(e => e.IsLastValue);

}

[Fact(Skip = "requires support for cs8 in roslyn scripting")]
Expand Down
2 changes: 2 additions & 0 deletions WorkspaceServer/Kernel/CSharpKernel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ private async Task HandleSubmitCode(
new ValueProduced(
std,
codeSubmission,
false,
formattedValues));
}
if (HasReturnValue)
Expand All @@ -194,6 +195,7 @@ private async Task HandleSubmitCode(
new ValueProduced(
_scriptState.ReturnValue,
codeSubmission,
true,
formattedValues));
}

Expand Down