Several refactorings to rework the internal Completion List API #3955
Conversation
* Convert the ICompletionProvider interface to a CompletionListProvider abstract class. * Rename CompletionItemGroup to CompletionList * Introduce a CompletionListContext type to be used by providers to specify the contents and details of CompletionLists with a RegisterCompletionListAsync() method. * Change GetGroupsAsync() method to GetCompletionListAsync(), which returns Task<CompletionList> rather than Task<IEnumerable<CompletionList>>. * Move code that merges, de-dupes and sorts completion lists from the controller into the completion service. This affected a few VB spell check unit tests which now change their ordering slightly.
DustinCampbell
added
Area-IDE
Concept-API
|
This is all part of the work for Issue #3538. There's much more to come yet. 😄 Adding @CyrusNajmabadi, @rchande, @Pilchie, @jasonmalinowski, @dpoeschl |
|
A VB spell check unit test failed because the suggestions do not have a stable sort. They're sorted by goodness but need to be further sorted by display text when the goodness is the same. I'll fix that up tomorrow. This shouldn't block reviewing the change though. |
|
|
||
| namespace Microsoft.CodeAnalysis.Editor.Extensibility.Completion | ||
| { | ||
| internal abstract class SnippetCompletionProvider : AbstractCompletionProvider, ISnippetCompletionProvider, ICustomCommitCompletionProvider |
There was a problem hiding this comment.
Hrmm.... How can this implement ISnippetCompletionProvider. I saw you removed it above... Or did it move? If so, odd that git didn't notice.
There was a problem hiding this comment.
Yeah, it moved down to Features. Looks different enough that git didn't call it a rename.
| throw new ArgumentNullException(nameof(item)); | ||
| } | ||
|
|
||
| _addCompletionItem(item); |
There was a problem hiding this comment.
Even if this is a struct, i don't think i see the need for this indirection. We could just move the immutablearraybuilder into this type, and have it internally exposed for the completion list infrastructure to get at the actual values.
This is willfuilly inconsistent with the methods in CodeFixProvider, CodeRefactoringProvider and DiagnosticAnalyzer becuase the name feels a bit better in this case. This might be changed following an API design review before being made public.
|
Added a few commits to address some of the CR feedback. I went ahead and made CompletionList.Items an |
|
Just please don't rename |
|
@sharwell -- that's extremely foul! 😄 🙈 FWIW, there isn't a plan to change that particular type ATM. Of course, that's the danger of using reflection to access internal stuff. We intend to expose several of our currently internal bits as public API and we reserve the right to change our internal APIs to better expose them. |
| /// </summary> | ||
| public static Task<IEnumerable<CompletionItemGroup>> GetCompletionItemGroupsAsync(Document document, int position, CompletionTriggerInfo triggerInfo, IEnumerable<ICompletionProvider> completionProviders = null, CancellationToken cancellationToken = default(CancellationToken)) | ||
| public static Task<CompletionList> GetCompletionListAsync(Document document, int position, CompletionTriggerInfo triggerInfo, IEnumerable<CompletionListProvider> completionProviders = null, CancellationToken cancellationToken = default(CancellationToken)) |
I realized that we can actually avoid reflection in this case by being clever a different way: EWSoftware/SHFB#146. |
Several refactorings to rework the internal Completion List API
ICompletionProviderinterface to aCompletionListProviderabstract class.CompletionItemGrouptoCompletionListCompletionListContexttype to be used by providers to specify the contents anddetails of CompletionLists with a
RegisterCompletionListAsync()method.GetGroupsAsync()method toGetCompletionListAsync(), which returnsTask<CompletionList>ratherthan
Task<IEnumerable<CompletionList>>.completion service. This affected a few VB spell check unit tests which now change their ordering
slightly.