Skip to content
This repository was archived by the owner on Oct 16, 2020. It is now read-only.

Switch to resolving expensive completionItem details #344

Merged
merged 9 commits into from
Sep 8, 2017
Merged

Switch to resolving expensive completionItem details #344

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a74c79e
fix: Move lookup of completion details to completionItem/resolve
tomv564 Sep 4, 2017
47b8bb0
fix: lint errors
tomv564 Sep 4, 2017
afa33f9
fix: Restrict CompletionItem.data, restore full test cases for resolv…
tomv564 Sep 4, 2017
ba9de9f
fix: Restore unresolved completion test with snippets
tomv564 Sep 5, 2017
90a6515
fix: Clean commented out code + unneeded documentation
tomv564 Sep 6, 2017
7fc166e
Merge branch 'master' into resolve-completions
felixfbecker Sep 6, 2017
5bfe836
refactor: use rx
felixfbecker Sep 8, 2017
9b2f9ea
style: don't compare with null
felixfbecker Sep 8, 2017
f5913ac
Merge branch 'master' into resolve-completions
felixfbecker Sep 8, 2017
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
159 changes: 91 additions & 68 deletions src/test/typescript-service-helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { applyReducer, Operation } from 'fast-json-patch';
import { IBeforeAndAfterContext, ISuiteCallbackContext, ITestCallbackContext } from 'mocha';
import * as sinon from 'sinon';
import * as ts from 'typescript';
import { CompletionItemKind, CompletionList, DiagnosticSeverity, InsertTextFormat, TextDocumentIdentifier, TextDocumentItem, WorkspaceEdit } from 'vscode-languageserver';
import { CompletionItem, CompletionItemKind, CompletionList, DiagnosticSeverity, InsertTextFormat, TextDocumentIdentifier, TextDocumentItem, WorkspaceEdit } from 'vscode-languageserver';
import { Command, Diagnostic, Hover, Location, SignatureHelp, SymbolInformation, SymbolKind } from 'vscode-languageserver-types';
import { LanguageClient, RemoteLanguageClient } from '../lang-handler';
import { DependencyReference, PackageInformation, ReferenceInformation, TextDocumentContentParams, WorkspaceFilesParams } from '../request-type';
Expand Down Expand Up @@ -2169,44 +2169,22 @@ export function describeTypeScriptService(createService: TypeScriptServiceFactor
// * the end of the snippet. Placeholders with equal identifiers are linked,
// * that is typing in one will update others too.
assert.equal(result.isIncomplete, false);
assert.sameDeepMembers(result.items, [
{
label: 'bar',
kind: CompletionItemKind.Method,
documentation: 'bar doc',
sortText: '0',
insertTextFormat: InsertTextFormat.Snippet,
insertText: 'bar(${1:num})',
detail: '(method) A.bar(num: number): number'
},
{
label: 'baz',
kind: CompletionItemKind.Method,
documentation: 'baz doc',
sortText: '0',
insertTextFormat: InsertTextFormat.Snippet,
insertText: 'baz(${1:num})',
detail: '(method) A.baz(num: number): string'
},
{
label: 'foo',
kind: CompletionItemKind.Method,
documentation: 'foo doc',
sortText: '0',
insertTextFormat: InsertTextFormat.Snippet,
insertText: 'foo()',
detail: '(method) A.foo(): void'
},
{
label: 'qux',
kind: CompletionItemKind.Property,
documentation: 'qux doc',
sortText: '0',
insertTextFormat: InsertTextFormat.Snippet,
insertText: 'qux',
detail: '(property) A.qux: number'
}
]);
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this assertion removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

Copy link
Contributor

Choose a reason for hiding this comment

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

I would leave this test as is (just change the objects) and add a separate test for the resolving


const barItem = result.items.find(i => i.label === 'bar') as CompletionItem;
const resolved: CompletionItem = await this.service
.completionItemResolve(barItem)
.reduce<Operation, CompletionItem>(applyReducer, null as any).toPromise();

assert.deepEqual(resolved, {
label: 'bar',
kind: CompletionItemKind.Method,
documentation: 'bar doc',
insertText: 'bar(${1:num})',
insertTextFormat: InsertTextFormat.Snippet,
sortText: '0',
detail: '(method) A.bar(num: number): number'
});
Copy link
Contributor

Choose a reason for hiding this comment

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

this would better be a second test


});
});

Expand Down Expand Up @@ -2259,42 +2237,77 @@ export function describeTypeScriptService(createService: TypeScriptServiceFactor
assert.equal(result.isIncomplete, false);
assert.sameDeepMembers(result.items, [
{
data: {
entryName: 'bar',
offset: 188,
uri: rootUri + 'a.ts'
},
label: 'bar',
kind: CompletionItemKind.Method,
documentation: 'bar doc',
insertText: 'bar',
insertTextFormat: InsertTextFormat.PlainText,
sortText: '0',
detail: '(method) A.bar(): number'
// documentation: 'bar doc',
// insertText: 'bar',
// insertTextFormat: InsertTextFormat.PlainText,
sortText: '0'
// detail: '(method) A.bar(): number'
},
{
data: {
entryName: 'baz',
offset: 188,
uri: rootUri + 'a.ts'
},
label: 'baz',
kind: CompletionItemKind.Method,
documentation: 'baz doc',
insertText: 'baz',
insertTextFormat: InsertTextFormat.PlainText,
sortText: '0',
detail: '(method) A.baz(): string'
// documentation: 'baz doc',
// insertText: 'baz',
// insertTextFormat: InsertTextFormat.PlainText,
sortText: '0'
// detail: '(method) A.baz(): string'
},
{
data: {
entryName: 'foo',
offset: 188,
uri: rootUri + 'a.ts'
},
label: 'foo',
kind: CompletionItemKind.Method,
documentation: 'foo doc',
insertText: 'foo',
insertTextFormat: InsertTextFormat.PlainText,
sortText: '0',
detail: '(method) A.foo(): void'
// documentation: 'foo doc',
// insertText: 'foo',
// insertTextFormat: InsertTextFormat.PlainText,
sortText: '0'
// detail: '(method) A.foo(): void'
},
{
data: {
entryName: 'qux',
offset: 188,
uri: rootUri + 'a.ts'
},
label: 'qux',
kind: CompletionItemKind.Property,
documentation: 'qux doc',
insertText: 'qux',
insertTextFormat: InsertTextFormat.PlainText,
sortText: '0',
detail: '(property) A.qux: number'
// documentation: 'qux doc',
// insertText: 'qux',
// insertTextFormat: InsertTextFormat.PlainText,
sortText: '0'
// detail: '(property) A.qux: number'
}
]);

const barItem = result.items.find(i => i.label === 'bar') as CompletionItem;
const resolved: CompletionItem = await this.service
.completionItemResolve(barItem)
.reduce<Operation, CompletionItem>(applyReducer, null as any).toPromise();

assert.deepEqual(resolved, {
label: 'bar',
kind: CompletionItemKind.Method,
documentation: 'bar doc',
insertText: 'bar',
insertTextFormat: InsertTextFormat.PlainText,
sortText: '0',
detail: '(method) A.bar(): number'
});
});

it('produces completions for imported symbols', async function (this: TestContext & ITestCallbackContext) {
Expand All @@ -2310,12 +2323,17 @@ export function describeTypeScriptService(createService: TypeScriptServiceFactor
assert.deepEqual(result, {
isIncomplete: false,
items: [{
data: {
entryName: 'd',
offset: 32,
uri: rootUri + 'uses-import.ts'
},
label: 'd',
kind: CompletionItemKind.Function,
documentation: 'd doc',
insertText: 'd',
insertTextFormat: InsertTextFormat.PlainText,
detail: 'function d(): void',
// documentation: 'd doc',
// insertText: 'd',
// insertTextFormat: InsertTextFormat.PlainText,
// detail: 'function d(): void',
sortText: '0'
}]
});
Expand All @@ -2333,13 +2351,18 @@ export function describeTypeScriptService(createService: TypeScriptServiceFactor
assert.deepEqual(result, {
isIncomplete: false,
items: [{
data: {
entryName: 'bar',
offset: 51,
uri: rootUri + 'uses-reference.ts'
},
label: 'bar',
kind: CompletionItemKind.Interface,
documentation: 'bar doc',
insertText: 'bar',
insertTextFormat: InsertTextFormat.PlainText,
sortText: '0',
detail: 'interface foo.bar'
// documentation: 'bar doc',
// insertText: 'bar',
// insertTextFormat: InsertTextFormat.PlainText,
sortText: '0'
// detail: 'interface foo.bar'
}]
});
});
Expand Down
71 changes: 51 additions & 20 deletions src/typescript-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1022,33 +1022,64 @@ export class TypeScriptService {
if (entry.sortText) {
item.sortText = entry.sortText;
}
const details = configuration.getService().getCompletionEntryDetails(fileName, offset, entry.name);
if (details) {
item.documentation = ts.displayPartsToString(details.documentation);
item.detail = ts.displayPartsToString(details.displayParts);
if (this.supportsCompletionWithSnippets) {
item.insertTextFormat = InsertTextFormat.Snippet;
if (entry.kind === 'property') {
item.insertText = details.name;
} else {
const parameters = details.displayParts
.filter(p => p.kind === 'parameterName')
.map((p, i) => '${' + `${i + 1}:${p.text}` + '}');
const paramString = parameters.join(', ');
item.insertText = details.name + `(${paramString})`;
}
} else {
item.insertTextFormat = InsertTextFormat.PlainText;
item.insertText = details.name;
}
}

// context for future resolve requests:
item.data = {
uri,
offset,
entryName: entry.name
};
Copy link
Contributor

Choose a reason for hiding this comment

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

could you define an interface for this / add this to a custom CompletionItem interface in request-type?


return { op: 'add', path: '/items/-', value: item } as Operation;
})
.startWith({ op: 'add', path: '/isIncomplete', value: false } as Operation);
})
.startWith({ op: 'add', path: '', value: { isIncomplete: true, items: [] } as CompletionList } as Operation);
}

/**
* The completionItem/resolve request is used to fill in additional details from an incomplete
* CompletionItem returned from the textDocument/completions call.
*
* @return Observable of JSON Patches that build a `CompletionItem` result
*/
completionItemResolve(item: CompletionItem, span = new Span()): Observable<Operation> {
const {uri, offset, entryName} = item.data;
const fileName: string = uri2path(uri);
return this.projectManager.ensureReferencedFiles(uri, undefined, undefined, span)
.toArray()
.map(() => {

const configuration = this.projectManager.getConfiguration(fileName);
configuration.ensureBasicFiles(span);

const details = configuration.getService().getCompletionEntryDetails(fileName, offset, entryName);
if (details) {
item.documentation = ts.displayPartsToString(details.documentation);
item.detail = ts.displayPartsToString(details.displayParts);
if (this.supportsCompletionWithSnippets) {
item.insertTextFormat = InsertTextFormat.Snippet;
if (details.kind === 'method' || details.kind === 'function') {
const parameters = details.displayParts
.filter(p => p.kind === 'parameterName')
.map((p, i) => '${' + `${i + 1}:${p.text}` + '}');
const paramString = parameters.join(', ');
item.insertText = details.name + `(${paramString})`;
} else {
item.insertText = details.name;

}
} else {
item.insertTextFormat = InsertTextFormat.PlainText;
item.insertText = details.name;
}
delete item.data;
Copy link
Contributor

Choose a reason for hiding this comment

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

delete is slow, is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

}
return item;
})
.map(completionItem => ({ op: 'add', path: '', value: completionItem }) as Operation);
}

/**
* The signature help request is sent from the client to the server to request signature
* information at a given cursor position.
Expand Down