introduce CompletionContext.Empty, remove Symbol.GetCompletions(void) #1954
Conversation
* cache it so it does not get created more than once * make it public so it can be reused by our users who write tests * use it places where it can improve performance (including benchmarks, which should not include this allocation)
| 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; |
There was a problem hiding this comment.
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.
| public IEnumerable<CompletionItem> GetCompletions() => | ||
| GetCompletions(CompletionContext.Empty()); |
There was a problem hiding this comment.
This feels like an unnecessary breaking change to me.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ok that's fair, if is proven later on that this method is valuable, we can bring it back.
introduce static
CompletionContext.Empty:remove Symbol.GetCompletions(void): mostly to simplify
Symbol@jonsequitur if we don't want to delete
Symbol.GetCompletions(void)we should keep caching the empty completion context (perf gains inHelpBuilderandParseResultVisitor)