Skip to content

Fixes for a couple of magic command completion cases #3709

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

Merged
merged 3 commits into from
Oct 11, 2024
Merged

Fixes for a couple of magic command completion cases #3709

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5c22579
fix a couple of intermittent test failures based on clock time
jonsequitur Oct 11, 2024
a530ddb
fix a couple of completion bugs
jonsequitur Oct 11, 2024
ab27820
comment cleanup
jonsequitur Oct 11, 2024
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
10 changes: 5 additions & 5 deletions src/Microsoft.DotNet.Interactive.Http.Tests/HttpKernelTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1439,7 +1439,7 @@ public async Task can_bind_local_datetime_with_arguments()
var result = await kernel.SendAsync(new SubmitCode(code));
result.Events.Should().NotContainErrors();

var currentDate = DateTime.UtcNow.ToString("yyyy-MM-dd");
var currentDate = DateTime.Now.ToString("yyyy-MM-dd");
var bodyAsString = await request.Content.ReadAsStringAsync();
var readDateSubstring = bodyAsString.Split(":").Last().Trim().Substring(1);
var readDateValue = readDateSubstring.Substring(0, readDateSubstring.IndexOf("\""));
Expand Down Expand Up @@ -1472,11 +1472,11 @@ public async Task can_bind_local_datetime_with_iso_format()
var result = await kernel.SendAsync(new SubmitCode(code));
result.Events.Should().NotContainErrors();

var currentDate = DateTimeOffset.UtcNow;
var currentDateTime = DateTimeOffset.Now;
var bodyAsString = await request.Content.ReadAsStringAsync();
var readDateSubstring = bodyAsString.Split("\"local_custom_date\" : ").Last().Trim().Substring(1);
var readDateValue = readDateSubstring.Substring(0, readDateSubstring.IndexOf("\""));
DateTimeOffset.Parse(readDateValue).Should().BeCloseTo(currentDate, TimeSpan.FromSeconds(10));
DateTimeOffset.Parse(readDateValue).Should().BeCloseTo(currentDateTime, TimeSpan.FromSeconds(10));
}

[Fact]
Expand Down Expand Up @@ -1505,7 +1505,7 @@ public async Task can_bind_local_datetime_with_additonal_spaces()
var result = await kernel.SendAsync(new SubmitCode(code));
result.Events.Should().NotContainErrors();

var currentDate = DateTimeOffset.UtcNow.AddDays(-1);
var currentDate = DateTimeOffset.Now.AddDays(-1);
var bodyAsString = await request.Content.ReadAsStringAsync();
var readDateSubstring = bodyAsString.Split("\"local_custom_date\" : ").Last().Trim().Substring(1);
var readDateValue = readDateSubstring.Substring(0, readDateSubstring.IndexOf("\""));
Expand Down Expand Up @@ -1538,7 +1538,7 @@ public async Task can_bind_local_datetime_with_rfc_format()
var result = await kernel.SendAsync(new SubmitCode(code));
result.Events.Should().NotContainErrors();

var currentDate = DateTimeOffset.UtcNow;
var currentDate = DateTimeOffset.Now;
var bodyAsString = await request.Content.ReadAsStringAsync();
var readDateSubstring = bodyAsString.Split("\"local_custom_date\" : ").Last().Trim().Substring(1);
var readDateValue = readDateSubstring.Substring(0, readDateSubstring.IndexOf("\""));
Expand Down
43 changes: 35 additions & 8 deletions src/Microsoft.DotNet.Interactive.Parsing.Tests/PolyglotParserConfigurationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,17 +66,17 @@ internal static PolyglotParserConfiguration GetDefaultConfiguration(string defau
},
new KernelActionDirective("#!connect")
{
Parameters =
{
new KernelDirectiveParameter("--kernel-name")
},
Subcommands =
{
new KernelActionDirective("named-pipe")
{
Parameters =
{
new("--pipe-name")
{
Required = true
},
new("--kernel-name")
{
Required = true
}
Expand All @@ -88,36 +88,63 @@ internal static PolyglotParserConfiguration GetDefaultConfiguration(string defau
{
new("--working-directory"),
new("--command"),
new("--kernel-host")
new("--kernel-host"),
new("--kernel-name")
{
Required = true
}
}
},
new KernelActionDirective("signalr")
{
Parameters =
{
new("--hub-url")
{
Required = true
},
new("--kernel-name")
{
Required = true
}
}
},
new KernelActionDirective("jupyter"),
new KernelActionDirective("jupyter")
{
Parameters =
{
new("--kernel-name")
{
Required = true
},
new("--kernel-spec", "The kernel spec to connect to")
{
Required = true
},
new("--init-script", "Script to run on kernel initialization")
}
},
new KernelActionDirective("mssql")
{
Parameters =
{
new KernelDirectiveParameter("--connection-string")
{
AllowImplicitName = true,
Required = true
Required = true,
TypeHint = "connectionstring-mssql"
},
new KernelDirectiveParameter("--create-dbcontext")
{
Flag = true
},
new("--kernel-name")
{
Required = true
}
},
KernelCommandType = typeof(ConnectMsSql)
},
}
}
},
new KernelSpecifierDirective("#!javascript", "javascript")
Expand Down
3 changes: 2 additions & 1 deletion src/Microsoft.DotNet.Interactive.Parsing.Tests/PolyglotSyntaxParserTests.Completions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ public class Directives
[Theory]
[InlineData("#!$$", new[]{"#!connect", "#!set", "#!who"})]
[InlineData("#!conn$$", new[]{"#!connect"})]
public async Task produce_completions_for_partiaL_text(string markupCode, string[] expectedCompletions)
public async Task produce_completions_for_partial_text(string markupCode, string[] expectedCompletions)
{
MarkupTestFile.GetPosition(markupCode, out var code, out var position);

Expand Down Expand Up @@ -203,6 +203,7 @@ public async Task produce_completions_for_parameter_names(string markupCode)
[InlineData("#!connect mssql $$")]
[InlineData("#!connect mssql $$")]
[InlineData("#!connect mssql --connection-string @input $$")]
[InlineData("#!connect mssql --connection-string abc $$")]
[InlineData("#!connect mssql --create-dbcontext $$ @input")]
public async Task do_not_produce_completions_for_sibling_subcommands(string markupCode)
{
Expand Down
45 changes: 31 additions & 14 deletions src/Microsoft.DotNet.Interactive/Parsing/DirectiveNode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,7 +463,7 @@ public async Task<IReadOnlyList<CompletionItem>> GetCompletionsAtPositionAsync(i
completions = completions.Where(c => c.AssociatedSymbol is KernelDirective).ToList();
}

return FilterOutParametersWithMaxOccurrencesReached(completions);
return FilterOutCompletionsWithMaxOccurrencesReached(completions);
}

if (currentToken is not { Kind: TokenKind.Whitespace })
Expand All @@ -488,22 +488,33 @@ public async Task<IReadOnlyList<CompletionItem>> GetCompletionsAtPositionAsync(i
var completions = await parameter.GetValueCompletionsAsync();
return completions;
}
else

var parentDirectiveNode = pn.Ancestors().OfType<DirectiveNode>().First();

if (parentDirectiveNode.TryGetDirective(out var parentDirective))
{
var parentDirectiveNode = pn.Ancestors().OfType<DirectiveNode>().First();
if (parentDirectiveNode.TryGetDirective(out var parentDirective))
// Since flags have no child nodes, we can return all completions for the parent directive.
List<CompletionItem> completions = [];
completions.AddRange(await parentDirective.GetChildCompletionsAsync());

if (parentDirective is KernelActionDirective parentActionDirective)
{
// Since flags have no child nodes, we can return all completions for the parent directive.
var completions = await parentDirective.GetChildCompletionsAsync();
// Include parameter names from the subcommand
var subcommandDirective = parentActionDirective.Subcommands.FirstOrDefault(s => s.Name == parentDirectiveNode.SubcommandNode?.NameNode?.Text);

// If there's already a subcommand node, then skip subcommand completions
if (parentDirectiveNode.SubcommandNode is not null)
if (subcommandDirective is not null)
{
completions = completions.Where(c => c.AssociatedSymbol is not KernelActionDirective).ToList();
completions.AddRange(subcommandDirective.Parameters.Select(p => new CompletionItem(p.Name, WellKnownTags.Property)
{
AssociatedSymbol = p,
Documentation = p.Description
}));
}

return completions.ToArray();
}

completions = FilterOutCompletionsWithMaxOccurrencesReached(completions);

return completions.ToArray();
}
}

Expand Down Expand Up @@ -560,7 +571,7 @@ public async Task<IReadOnlyList<CompletionItem>> GetCompletionsAtPositionAsync(i
// This could also be a partial subcommand, so...
var completions = await directive.GetChildCompletionsAsync();

return FilterOutParametersWithMaxOccurrencesReached(completions);
return FilterOutCompletionsWithMaxOccurrencesReached(completions);
}
}

Expand Down Expand Up @@ -609,7 +620,7 @@ CompletionItem[] GetCompletionsForPartialDirective()
.ToArray();
}

List<CompletionItem> FilterOutParametersWithMaxOccurrencesReached(IReadOnlyList<CompletionItem> completions)
List<CompletionItem> FilterOutCompletionsWithMaxOccurrencesReached(IReadOnlyList<CompletionItem> completions)
{
var filteredCompletions = new List<CompletionItem>();

Expand All @@ -619,14 +630,20 @@ List<CompletionItem> FilterOutParametersWithMaxOccurrencesReached(IReadOnlyList<
.GroupBy(n => n.NameNode!.Text)
.ToDictionary(g => g.Key, g => g.Count());

var subcommandWasProvided = DescendantNodesAndTokens().OfType<DirectiveSubcommandNode>().Any();

for (var i = 0; i < completions.Count; i++)
{
var completion = completions[i];
if (completion.AssociatedSymbol is not KernelDirectiveParameter p ||
!parametersProvided.TryGetValue(completion.InsertText, out var count) ||
count < p.MaxOccurrences)
{
filteredCompletions.Add(completion);
if (!subcommandWasProvided ||
completion.AssociatedSymbol is not KernelActionDirective)
{
filteredCompletions.Add(completion);
}
}
}

Expand Down
3 changes: 0 additions & 3 deletions src/Microsoft.DotNet.Interactive/Parsing/SubmissionParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -574,7 +574,6 @@ private async Task<ExpressionBindingResult> RequestMultipleInputsIfAppropriate(
DirectiveExpressionNode expressionNode,
string targetKernelName)
{
// FIX: (RequestSingleValueFromKernel) make sure this is not dispatched on language service commands
if (!(expressionNode
.Ancestors()
.OfType<DirectiveNode>()
Expand Down Expand Up @@ -629,8 +628,6 @@ sourceKernel is not null &&
bool allowByRef,
DirectiveExpressionNode expressionNode)
{
// FIX: (RequestSingleValueFromKernel) make sure this is not dispatched on language service commands

var requestValue = new RequestValue(sourceValueName, mimeType: mimeType, targetKernelName: sourceKernelName);

var result = await destinationKernel.RootKernel.SendAsync(requestValue);
Expand Down
1 change: 0 additions & 1 deletion src/Microsoft.DotNet.Interactive/ValueSharing/SetDirectiveCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ public static Task<KernelCommand> TryParseSetDirectiveCommandAsync(

if (bindingResult.InputsProduced.TryGetValue("--value", out var inputProduced))
{
// FIX: (TryParseSetDirectiveCommandAsync) can this be a BoundValue and remove InputsProduced property?
if (((RequestInput)inputProduced.Command).IsPassword)
{
command.ReferenceValue = new PasswordString(inputProduced.Value);
Expand Down