Skip to content

introduce CompletionContext.Empty, remove Symbol.GetCompletions(void) #1954

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 4 commits into from
Dec 14, 2022
Merged

introduce CompletionContext.Empty, remove Symbol.GetCompletions(void) #1954

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
31dfad1
introduce static CompletionContext.Empty:
adamsitnik Nov 8, 2022
3b25f1a
remove Symbol.GetCompletions(void)
adamsitnik Nov 8, 2022
5f12a6f
fix the warning
adamsitnik Nov 8, 2022
2eeb0d1
argument is never null here?
adamsitnik Nov 8, 2022
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
2 changes: 1 addition & 1 deletion ...ty.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,6 @@ System.CommandLine
public System.Boolean IsHidden { get; set; }
public System.String Name { get; set; }
public System.Collections.Generic.IEnumerable<Symbol> Parents { get; }
public System.Collections.Generic.IEnumerable<System.CommandLine.Completions.CompletionItem> GetCompletions()
public System.Collections.Generic.IEnumerable<System.CommandLine.Completions.CompletionItem> GetCompletions(System.CommandLine.Completions.CompletionContext context)
public System.String ToString()
System.CommandLine.Binding
Expand All @@ -275,6 +274,7 @@ System.CommandLine.Binding
public System.Boolean TryGetValue(IValueDescriptor valueDescriptor, BindingContext bindingContext, ref System.Object& boundValue)
System.CommandLine.Completions
public abstract class CompletionContext
public static CompletionContext Empty { get; }
public System.CommandLine.ParseResult ParseResult { get; }
public System.String WordToComplete { get; }
public class CompletionItem
Expand Down
3 changes: 2 additions & 1 deletion src/System.CommandLine.Benchmarks/CommandLine/Perf_Suggestions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System.Collections.Generic;
using System.CommandLine.Completions;
using System.Linq;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Engines;
Expand Down Expand Up @@ -43,7 +44,7 @@ public void Setup_FromSymbol()
[Benchmark]
public void SuggestionsFromSymbol()
{
_testSymbol.GetCompletions().Consume(new Consumer());
_testSymbol.GetCompletions(CompletionContext.Empty).Consume(new Consumer());
}

[GlobalSetup(Target = nameof(SuggestionsFromParseResult))]
Expand Down
21 changes: 11 additions & 10 deletions src/System.CommandLine.Tests/CompletionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// 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.CommandLine.Completions;
using System.CommandLine.Parsing;
using System.CommandLine.Tests.Utility;
using System.IO;
Expand All @@ -27,7 +28,7 @@ public void Option_GetCompletions_returns_argument_completions_if_configured()
var option = new Option<string>("--hello")
.AddCompletions("one", "two", "three");

var completions = option.GetCompletions();
var completions = option.GetCompletions(CompletionContext.Empty);

completions
.Select(item => item.Label)
Expand All @@ -45,7 +46,7 @@ public void Command_GetCompletions_returns_available_option_aliases()
new Option<string>("--three", "option three")
};

var completions = command.GetCompletions();
var completions = command.GetCompletions(CompletionContext.Empty);

completions
.Select(item => item.Label)
Expand All @@ -69,7 +70,7 @@ public void Command_GetCompletions_returns_available_option_aliases_for_global_o

rootCommand.AddGlobalOption(new Option<string>("--three", "option three"));

var completions = subcommand.GetCompletions();
var completions = subcommand.GetCompletions(CompletionContext.Empty);

completions
.Select(item => item.Label)
Expand All @@ -87,7 +88,7 @@ public void Command_GetCompletions_returns_available_subcommands()
new Command("three")
};

var completions = command.GetCompletions();
var completions = command.GetCompletions(CompletionContext.Empty);

completions
.Select(item => item.Label)
Expand All @@ -104,7 +105,7 @@ public void Command_GetCompletions_returns_available_subcommands_and_option_alia
new Option<string>("--option")
};

var completions = command.GetCompletions();
var completions = command.GetCompletions(CompletionContext.Empty);

completions.Select(item => item.Label)
.Should()
Expand All @@ -125,7 +126,7 @@ public void Command_GetCompletions_returns_available_subcommands_and_option_alia
}
};

var completions = command.GetCompletions();
var completions = command.GetCompletions(CompletionContext.Empty);

completions.Select(item => item.Label)
.Should()
Expand All @@ -142,7 +143,7 @@ public void Command_GetCompletions_without_text_to_match_orders_alphabetically()
new Command("andmyothersubcommand"),
};

var completions = command.GetCompletions();
var completions = command.GetCompletions(CompletionContext.Empty);

completions
.Select(item => item.Label)
Expand All @@ -158,7 +159,7 @@ public void Command_GetCompletions_does_not_return_argument_names()
new Argument<string>("the-argument")
};

var completions = command.GetCompletions();
var completions = command.GetCompletions(CompletionContext.Empty);

completions
.Select(item => item.Label)
Expand Down Expand Up @@ -921,7 +922,7 @@ public void Completions_for_options_provide_a_description()
var description = "The option before -y.";
var option = new Option<string>("-x", description);

var completions = new RootCommand { option }.GetCompletions();
var completions = new RootCommand { option }.GetCompletions(CompletionContext.Empty);

completions.Should().ContainSingle()
.Which
Expand All @@ -936,7 +937,7 @@ public void Completions_for_subcommands_provide_a_description()
var description = "The description for the subcommand";
var subcommand = new Command("-x", description);

var completions = new RootCommand { subcommand }.GetCompletions();
var completions = new RootCommand { subcommand }.GetCompletions(CompletionContext.Empty);

completions.Should().ContainSingle()
.Which
Expand Down
8 changes: 7 additions & 1 deletion src/System.CommandLine/Completions/CompletionContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ namespace System.CommandLine.Completions
/// </summary>
public abstract class CompletionContext
{
private static CompletionContext? _empty;

internal CompletionContext(ParseResult parseResult, string wordToComplete)
{
ParseResult = parseResult;
Expand All @@ -23,7 +25,11 @@ internal CompletionContext(ParseResult parseResult, string wordToComplete)
/// The parse result for which completions are being requested.
public ParseResult ParseResult { get; }

internal static CompletionContext Empty() => new TokenCompletionContext(ParseResult.Empty());
/// <summary>
/// Gets an empty CompletionContext.
/// </summary>
/// <remarks>Can be used for testing purposes.</remarks>
public static CompletionContext Empty => _empty ??= new TokenCompletionContext(ParseResult.Empty());

/// <summary>
/// Gets the text to be matched for completion, which can be used to filter a list of completions.
Expand Down
9 changes: 4 additions & 5 deletions src/System.CommandLine/Help/HelpBuilder.Default.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,10 @@ public static string GetArgumentUsageLabel(Argument argument)
}

string firstColumn;
var completions = (argument is { } a
? a.GetCompletions()
: Array.Empty<CompletionItem>())
.Select(item => item.Label)
.ToArray();
var completions = argument
.GetCompletions(CompletionContext.Empty)
.Select(item => item.Label)
.ToArray();

var arg = argument;
var helpName = arg?.HelpName ?? string.Empty;
Comment on lines 66 to 73
Copy link

@KalleOlaviNiemitalo KalleOlaviNiemitalo Nov 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

There is a performance issue dotnet/sdk#27374 (comment) for this method; not changed in this PR, but maybe you'd like to tackle that too.

Filed as #2038.

Expand Down
3 changes: 2 additions & 1 deletion src/System.CommandLine/Parsing/ParseResultVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.Collections.Generic;
using System.CommandLine.Binding;
using System.CommandLine.Completions;
using System.CommandLine.Help;
using System.Linq;

Expand Down Expand Up @@ -522,7 +523,7 @@ private void ValidateAndConvertArgumentResult(ArgumentResult argumentResult)
{
if (argument.FirstParent?.Symbol is Option option)
{
var completions = option.GetCompletions().ToArray();
var completions = option.GetCompletions(CompletionContext.Empty).ToArray();

if (completions.Length > 0)
{
Expand Down
4 changes: 0 additions & 4 deletions src/System.CommandLine/Symbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,6 @@ public IEnumerable<Symbol> Parents
/// <summary>
/// Gets completions for the symbol.
/// </summary>
public IEnumerable<CompletionItem> GetCompletions() =>
GetCompletions(CompletionContext.Empty());
Comment on lines -83 to -84
Copy link
Member

Choose a reason for hiding this comment

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

This feels like an unnecessary breaking change to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

This feels like an unnecessary breaking change to me.

The main point of my changes is simplification of the APIs which should help us get them approved and moved into BCL.

One of the users raised a concern about removing this method: #1891 (comment) but by exposing CompletionContext.Empty we are going to make it testable.

Copy link
Member

Choose a reason for hiding this comment

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

Ok that's fair, if is proven later on that this method is valuable, we can bring it back.


/// <inheritdoc />
public abstract IEnumerable<CompletionItem> GetCompletions(CompletionContext context);

/// <inheritdoc/>
Expand Down