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

Jump to bottom

fix #2095: remove SubmitCode.SubmissionType #3174

Merged
merged 1 commit into from
Sep 14, 2023
Merged

fix #2095: remove SubmitCode.SubmissionType #3174

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 1 addition & 5 deletions ....ApiCompatibility.Tests/ApiCompatibilityTests.Interactive_api_is_not_changed.approved.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,13 +441,9 @@ Microsoft.DotNet.Interactive.Commands
public Microsoft.DotNet.Interactive.FormattedValue FormattedValue { get;}
public System.String Name { get;}
public System.Object Value { get;}
public enum SubmissionType : System.Enum, System.IComparable, System.IConvertible, System.IFormattable
Run=0
Diagnose=1
public class SubmitCode : KernelCommand, System.IEquatable<KernelCommand>
.ctor(System.String code, System.String targetKernelName = null, SubmissionType submissionType = Run)
.ctor(System.String code, System.String targetKernelName = null)
public System.String Code { get;}
public SubmissionType SubmissionType { get;}
public System.String ToString()
public class UpdateDisplayedValue : KernelCommand, System.IEquatable<KernelCommand>
.ctor(Microsoft.DotNet.Interactive.FormattedValue formattedValue, System.String valueId)
Expand Down
5 changes: 0 additions & 5 deletions src/Microsoft.DotNet.Interactive.CSharp/CSharpKernel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,11 +290,6 @@ async Task IKernelCommandHandler<SubmitCode>.HandleAsync(SubmitCode submitCode,
context.Publish(new IncompleteCodeSubmissionReceived(submitCode));
}

if (submitCode.SubmissionType == SubmissionType.Diagnose)
{
return;
}

Exception exception = null;
string message = null;

Expand Down
3 changes: 0 additions & 3 deletions ...et.Interactive.Jupyter.Tests/Microsoft.DotNet.Interactive.Jupyter.Tests.v3.ncrunchproject
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,6 @@
<FixtureTestSelector>
<FixtureName>Microsoft.DotNet.Interactive.Jupyter.Tests.InterruptRequestHandlerTests</FixtureName>
</FixtureTestSelector>
<FixtureTestSelector>
<FixtureName>Microsoft.DotNet.Interactive.Jupyter.Tests.IsCompleteRequestHandlerTests</FixtureName>
</FixtureTestSelector>
<FixtureTestSelector>
<FixtureName>Microsoft.DotNet.Interactive.Jupyter.Tests.JupyterKernelTests</FixtureName>
</FixtureTestSelector>
Expand Down
16 changes: 11 additions & 5 deletions src/Microsoft.DotNet.Interactive.Jupyter/IsCompleteRequestHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
// Copyright (c) .NET Foundation and contributors. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System.Linq;
using System.Reactive.Concurrency;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis;
using Microsoft.DotNet.Interactive.Commands;
using Microsoft.DotNet.Interactive.Events;
using Microsoft.DotNet.Interactive.Jupyter.Protocol;
Expand All @@ -21,7 +23,7 @@ public async Task Handle(JupyterRequestContext context)
{
var isCompleteRequest = GetJupyterRequest(context);
var targetKernelName = context.GetKernelName();
var command = new SubmitCode(isCompleteRequest.Code, targetKernelName, submissionType: SubmissionType.Diagnose);
var command = new RequestDiagnostics(isCompleteRequest.Code, targetKernelName);

await SendAsync(context, command);
}
Expand All @@ -32,11 +34,15 @@ protected override void OnKernelEventReceived(
{
switch (@event)
{
case CompleteCodeSubmissionReceived _:
Reply(true, context.JupyterRequestMessageEnvelope, context.JupyterMessageSender);
case DiagnosticsProduced diagnosticsProduced:
shyamnamboodiripad marked this conversation as resolved.
Show resolved Hide resolved
if (diagnosticsProduced.Diagnostics.Any(d => d.Severity == DiagnosticSeverity.Error))
{
Reply(false, context.JupyterRequestMessageEnvelope, context.JupyterMessageSender);
}
break;
case IncompleteCodeSubmissionReceived _:
Reply(false, context.JupyterRequestMessageEnvelope, context.JupyterMessageSender);
Copy link
Contributor

Choose a reason for hiding this comment

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

Were the removed code paths pretty much unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're unused after this code change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not very familiar with the code, but the current class is called IsCompleteRequestHandler and I was a bit confused since it seemed like it was meant to check whether the complete submission has been received while now it checks whether or not the submission contains errors. Do we need to rename this for clarity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checking for errors is an implementation detail. This handler's purpose is the same as before, which is to inform the Jupyter console as to whether a code submission is "complete". This isn't a common use case for us in any event.


case CommandSucceeded _:
Reply(true, context.JupyterRequestMessageEnvelope, context.JupyterMessageSender);
break;
}
}
Expand Down
6 changes: 0 additions & 6 deletions src/Microsoft.DotNet.Interactive.PowerShell/PowerShellKernel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,12 +265,6 @@ async Task IKernelCommandHandler<SubmitCode>.HandleAsync(
return;
}

// Do nothing if we get a Diagnose type.
if (submitCode.SubmissionType == SubmissionType.Diagnose)
{
return;
}

if (context.CancellationToken.IsCancellationRequested)
{
context.Fail(submitCode, null, "Command cancelled");
Expand Down
1 change: 0 additions & 1 deletion ...nnection/SerializationTests.Command_contract_has_not_been_broken.approved.SubmitCode.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
"commandType": "SubmitCode",
"command": {
"code": "123",
"submissionType": "run",
"targetKernelName": "csharp",
"originUri": null,
"destinationUri": null
Expand Down
1 change: 0 additions & 1 deletion ...erializationTests.Event_contract_has_not_been_broken.approved.CodeSubmissionReceived.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
"commandType": "SubmitCode",
"command": {
"code": "123",
"submissionType": "run",
"targetKernelName": null,
"originUri": null,
"destinationUri": null
Expand Down
1 change: 0 additions & 1 deletion ...nection/SerializationTests.Event_contract_has_not_been_broken.approved.CommandFailed.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
"commandType": "SubmitCode",
"command": {
"code": "123",
"submissionType": "run",
"targetKernelName": null,
"originUri": null,
"destinationUri": null
Expand Down
1 change: 0 additions & 1 deletion ...tion/SerializationTests.Event_contract_has_not_been_broken.approved.CommandSucceeded.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
"commandType": "SubmitCode",
"command": {
"code": "123",
"submissionType": "run",
"targetKernelName": null,
"originUri": null,
"destinationUri": null
Expand Down
1 change: 0 additions & 1 deletion ...tionTests.Event_contract_has_not_been_broken.approved.CompleteCodeSubmissionReceived.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
"commandType": "SubmitCode",
"command": {
"code": "123",
"submissionType": "run",
"targetKernelName": null,
"originUri": null,
"destinationUri": null
Expand Down
1 change: 0 additions & 1 deletion ...n/SerializationTests.Event_contract_has_not_been_broken.approved.DiagnosticsProduced.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
"commandType": "SubmitCode",
"command": {
"code": "123",
"submissionType": "run",
"targetKernelName": null,
"originUri": null,
"destinationUri": null
Expand Down
1 change: 0 additions & 1 deletion ...erializationTests.Event_contract_has_not_been_broken.approved.DisplayedValueProduced.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
"commandType": "SubmitCode",
"command": {
"code": "b(\"hi!\")",
"submissionType": "run",
"targetKernelName": "csharp",
"originUri": null,
"destinationUri": null
Expand Down
1 change: 0 additions & 1 deletion ...SerializationTests.Event_contract_has_not_been_broken.approved.DisplayedValueUpdated.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
"commandType": "SubmitCode",
"command": {
"code": "b(\"hi!\")",
"submissionType": "run",
"targetKernelName": "csharp",
"originUri": null,
"destinationUri": null
Expand Down
1 change: 0 additions & 1 deletion ...nection/SerializationTests.Event_contract_has_not_been_broken.approved.ErrorProduced.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
"commandType": "SubmitCode",
"command": {
"code": "123",
"submissionType": "run",
"targetKernelName": null,
"originUri": null,
"destinationUri": null
Expand Down
1 change: 0 additions & 1 deletion ...onTests.Event_contract_has_not_been_broken.approved.IncompleteCodeSubmissionReceived.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
"commandType": "SubmitCode",
"command": {
"code": "123",
"submissionType": "run",
"targetKernelName": null,
"originUri": null,
"destinationUri": null
Expand Down
1 change: 0 additions & 1 deletion ...SerializationTests.Event_contract_has_not_been_broken.approved.KernelExtensionLoaded.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
"commandType": "SubmitCode",
"command": {
"code": "#r \"nuget:package\" ",
"submissionType": "run",
"targetKernelName": null,
"originUri": null,
"destinationUri": null
Expand Down
1 change: 0 additions & 1 deletion ...nnection/SerializationTests.Event_contract_has_not_been_broken.approved.PackageAdded.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
"commandType": "SubmitCode",
"command": {
"code": "#r \"nuget:ThePackage,1.2.3\"",
"submissionType": "run",
"targetKernelName": null,
"originUri": null,
"destinationUri": null
Expand Down
1 change: 0 additions & 1 deletion ...n/SerializationTests.Event_contract_has_not_been_broken.approved.ReturnValueProduced.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
"commandType": "SubmitCode",
"command": {
"code": "b(\"hi!\")",
"submissionType": "run",
"targetKernelName": "csharp",
"originUri": null,
"destinationUri": null
Expand Down
1 change: 0 additions & 1 deletion ...lizationTests.Event_contract_has_not_been_broken.approved.StandardErrorValueProduced.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
"commandType": "SubmitCode",
"command": {
"code": "123",
"submissionType": "run",
"targetKernelName": null,
"originUri": null,
"destinationUri": null
Expand Down
1 change: 0 additions & 1 deletion ...izationTests.Event_contract_has_not_been_broken.approved.StandardOutputValueProduced.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
"commandType": "SubmitCode",
"command": {
"code": "Console.Write(123);",
"submissionType": "run",
"targetKernelName": "csharp",
"originUri": null,
"destinationUri": null
Expand Down
10 changes: 5 additions & 5 deletions src/Microsoft.DotNet.Interactive.Tests/Connection/SerializationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ IEnumerable<KernelCommand> commands()

yield return new SendEditableCode("someKernelName", "code");

yield return new SubmitCode("123", "csharp", SubmissionType.Run);
yield return new SubmitCode("123", "csharp");

yield return new UpdateDisplayedValue(
new FormattedValue("text/html", "<b>hi!</b>"),
Expand Down Expand Up @@ -283,7 +283,7 @@ IEnumerable<KernelEvent> events()

yield return new DisplayedValueProduced(
new HtmlString("<b>hi!</b>"),
new SubmitCode("b(\"hi!\")", "csharp", SubmissionType.Run),
new SubmitCode("b(\"hi!\")", "csharp"),
new[]
{
new FormattedValue("text/html", "<b>hi!</b>"),
Expand All @@ -292,7 +292,7 @@ IEnumerable<KernelEvent> events()
yield return new DisplayedValueUpdated(
new HtmlString("<b>hi!</b>"),
"the-value-id",
new SubmitCode("b(\"hi!\")", "csharp", SubmissionType.Run),
new SubmitCode("b(\"hi!\")", "csharp"),
new[]
{
new FormattedValue("text/html", "<b>hi!</b>"),
Expand Down Expand Up @@ -361,7 +361,7 @@ IEnumerable<KernelEvent> events()

yield return new ReturnValueProduced(
new HtmlString("<b>hi!</b>"),
new SubmitCode("b(\"hi!\")", "csharp", SubmissionType.Run),
new SubmitCode("b(\"hi!\")", "csharp"),
new[]
{
new FormattedValue("text/html", "<b>hi!</b>"),
Expand Down Expand Up @@ -390,7 +390,7 @@ IEnumerable<KernelEvent> events()
});

yield return new StandardOutputValueProduced(
new SubmitCode("Console.Write(123);", "csharp", SubmissionType.Run),
new SubmitCode("Console.Write(123);", "csharp"),
new[]
{
new FormattedValue("text/plain", "123")
Expand Down
8 changes: 4 additions & 4 deletions src/Microsoft.DotNet.Interactive.Tests/LanguageKernelTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,18 +162,18 @@ protected virtual CSharpKernel CreateCSharpKernel()
});
}

public async Task SubmitCode(Kernel kernel, string[] submissions, SubmissionType submissionType = SubmissionType.Run)
public async Task SubmitCode(Kernel kernel, string[] submissions)
{
foreach (var submission in submissions)
{
var cmd = new SubmitCode(submission, submissionType: submissionType);
var cmd = new SubmitCode(submission);
await kernel.SendAsync(cmd);
}
}

public async Task<KernelCommandResult> SubmitCode(Kernel kernel, string submission, SubmissionType submissionType = SubmissionType.Run)
public async Task<KernelCommandResult> SubmitCode(Kernel kernel, string submission)
{
var command = new SubmitCode(submission, submissionType: submissionType);
var command = new SubmitCode(submission);
return await kernel.SendAsync(command);
}

Expand Down
26 changes: 9 additions & 17 deletions src/Microsoft.DotNet.Interactive.Tests/LanguageKernelTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -713,7 +713,7 @@ public async Task requested_diagnostics_does_not_execute_directives_handlers(Lan
[InlineData(Language.CSharp)]
[InlineData(Language.PowerShell)]
// no F# equivalent, because it doesn't have the concept of complete/incomplete submissions
public async Task it_can_analyze_incomplete_submissions(Language language)
public async Task it_acknowledges_receipt_of_incomplete_submissions(Language language)
{
var kernel = CreateKernel(language);

Expand All @@ -723,7 +723,7 @@ public async Task it_can_analyze_incomplete_submissions(Language language)
Language.PowerShell => "$a ="
};

await SubmitCode(kernel, source, submissionType: SubmissionType.Diagnose);
await SubmitCode(kernel, source);

KernelEvents
.Single(e => e is IncompleteCodeSubmissionReceived);
Expand All @@ -737,7 +737,7 @@ public async Task it_can_analyze_incomplete_submissions(Language language)
[InlineData(Language.CSharp)]
[InlineData(Language.PowerShell)]
// no F# equivalent, because it doesn't have the concept of complete/incomplete submissions
public async Task it_can_analyze_complete_submissions(Language language)
public async Task it_acknowledged_receipt_of_complete_submissions_having_return_value(Language language)
{
var kernel = CreateKernel(language);

Expand All @@ -747,12 +747,8 @@ public async Task it_can_analyze_complete_submissions(Language language)
Language.PowerShell => "25",
};

await SubmitCode(kernel, source, submissionType: SubmissionType.Diagnose);

KernelEvents
.Should()
.NotContain(e => e is ReturnValueProduced);

await SubmitCode(kernel, source);

KernelEvents
.Should()
.Contain(e => e is CompleteCodeSubmissionReceived);
Expand All @@ -761,20 +757,16 @@ public async Task it_can_analyze_complete_submissions(Language language)
[Theory]
[InlineData(Language.CSharp)]
// no F# equivalent, because it doesn't have the concept of complete/incomplete submissions
public async Task it_can_analyze_complete_stdio_submissions(Language language)
public async Task it_acknowledged_receipt_of_complete_stdio_submissions(Language language)
{
var kernel = CreateKernel(language);

var source = language switch
{
Language.CSharp => "Console.WriteLine(\"Hello\")"
Language.CSharp => "Console.WriteLine(\"Hello\");"
};

await SubmitCode(kernel, source, submissionType: SubmissionType.Diagnose);

KernelEvents
.Should()
.NotContain(e => e is StandardOutputValueProduced);
await SubmitCode(kernel, source);

KernelEvents
.Should()
Expand Down Expand Up @@ -819,7 +811,7 @@ public async Task it_does_not_return_a_result_for_a_statement(Language language)
Language.CSharp => "if (true) { }"
};

await SubmitCode(kernel, source, submissionType: SubmissionType.Run);
await SubmitCode(kernel, source);

KernelEvents
.Should()
Expand Down
11 changes: 0 additions & 11 deletions src/Microsoft.DotNet.Interactive/Commands/SubmissionType.cs
View file
Open in desktop

This file was deleted.

8 changes: 1 addition & 7 deletions src/Microsoft.DotNet.Interactive/Commands/SubmitCode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,22 +10,18 @@ public class SubmitCode : KernelCommand
{
public SubmitCode(
string code,
string targetKernelName = null,
SubmissionType submissionType = SubmissionType.Run) : base(targetKernelName)
string targetKernelName = null) : base(targetKernelName)
{
Code = code ?? throw new ArgumentNullException(nameof(code));
SubmissionType = submissionType;
}

internal SubmitCode(
LanguageNode languageNode,
SubmissionType submissionType = SubmissionType.Run,
KernelNameDirectiveNode kernelNameDirectiveNode = null)
: base(languageNode.Name)
{
Code = languageNode.Text;
LanguageNode = languageNode;
SubmissionType = submissionType;
KernelNameDirectiveNode = kernelNameDirectiveNode;
SchedulingScope = languageNode.CommandScope;

Expand All @@ -37,8 +33,6 @@ internal SubmitCode(

public string Code { get; internal set; }

public SubmissionType SubmissionType { get; }

public override string ToString() => $"{nameof(SubmitCode)}: {Code?.TruncateForDisplay()}";

internal LanguageNode LanguageNode { get; }
Expand Down
Loading
Loading