Switch to resolving expensive completionItem details #344
Conversation
Codecov Report
@@ Coverage Diff @@
## master #344 +/- ##
==========================================
+ Coverage 60.03% 60.16% +0.13%
==========================================
Files 14 14
Lines 2137 2149 +12
Branches 350 351 +1
==========================================
+ Hits 1283 1293 +10
- Misses 705 706 +1
- Partials 149 150 +1
Continue to review full report at Codecov.
|
| insertText: 'qux', | ||
| detail: '(property) A.qux: number' | ||
| } | ||
| ]); |
There was a problem hiding this comment.
why is this assertion removed?
There was a problem hiding this comment.
The snippets logic is only used when resolving completion items, and I thought it would be a lot of logic to resolve all the items from the initial response.
Now that I think of it, it's probably not that bad - I'll give it a try!
There was a problem hiding this comment.
I would leave this test as is (just change the objects) and add a separate test for the resolving
| insertTextFormat: InsertTextFormat.Snippet, | ||
| sortText: '0', | ||
| detail: '(method) A.bar(num: number): number' | ||
| }); |
There was a problem hiding this comment.
this would better be a second test
| uri, | ||
| offset, | ||
| entryName: entry.name | ||
| }; |
There was a problem hiding this comment.
could you define an interface for this / add this to a custom CompletionItem interface in request-type?
src/typescript-service.ts
Outdated
| item.insertTextFormat = InsertTextFormat.PlainText; | ||
| item.insertText = details.name; | ||
| } | ||
| delete item.data; |
There was a problem hiding this comment.
delete is slow, is this needed?
There was a problem hiding this comment.
It's not needed. It would be nice to reduce payload and not have to include the field in the asserts on resolved completion items (I wonder if the tests should even care at all what is in the data field?). Could we set to undefined, null, or should I just leave it be?
|
I didn't split the snippets test cases yet, and assigned undefined to the data field after resolving for now. Thanks for the comments! |
|
@felixfbecker: your review comments are addressed, commented code is removed. One snippet improvement was also included: |
Requesting extra completion details was found to be very expensive in #331, this PR will require clients to request these details via a separate request.
To do: