Skip to content
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.

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

Do not pre-filter completions from kernel #1022

Merged
merged 3 commits into from
Nov 26, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions packages/jupyterlab-lsp/jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,11 @@ const esModules = [
].join('|');

let local = {
globals: { 'ts-jest': { tsconfig: 'tsconfig.json' } },
testRegex: `.*\.spec\.tsx?$`,
transformIgnorePatterns: [`/node_modules/(?!${esModules}).*`],
testLocationInResults: true,
transform: {
'\\.(ts|tsx)?$': 'ts-jest',
'\\.(ts|tsx)?$': ['ts-jest', { tsconfig: 'tsconfig.json' }],
'\\.(js|jsx)?$': './transform.js',
'\\.svg$': '@jupyterlab/testing/lib/jest-raw-loader.js'
},
Expand Down
47 changes: 46 additions & 1 deletion packages/jupyterlab-lsp/src/features/completion/model.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ describe('LSPCompleterModel', () => {
function createDummyItem(
match: lsProtocol.CompletionItem,
type: string = 'dummy',
source: string = 'lsp'
source: string = 'LSP'
) {
return new CompletionItem({
type,
Expand Down Expand Up @@ -80,6 +80,51 @@ describe('LSPCompleterModel', () => {
expect(sortedItems.map(item => item.sortText)).toEqual(['a', 'b', 'c']);
});

describe('pre-filtering', () => {
beforeEach(() => {
// order of cursor/current matters
model.current = model.original = {
text: 'a',
line: 0,
column: 1
};
model.cursor = { start: 0, end: 1 };
});

const prefixA = createDummyItem({
label: 'a'
});
const prefixB = createDummyItem({
label: 'b'
});
const prefixBButNotLSP = createDummyItem(
{
label: 'b'
},
'dummy',
'not LSP'
);

it('filters out non-matching LSP completions', () => {
model.setCompletionItems([prefixA, prefixB]);
let items = model.completionItems();
expect(items.map(item => item.insertText)).toEqual(['a']);
});

it('does not filter out non LSP completions', () => {
model.setCompletionItems([prefixA, prefixBButNotLSP]);
let items = model.completionItems();
expect(items.map(item => item.insertText)).toEqual(['a', 'b']);
});

it('does not filter out when turned off', () => {
model.setCompletionItems([prefixA, prefixB]);
model.settings.preFilterMatches = false;
let items = model.completionItems();
expect(items.map(item => item.insertText)).toEqual(['a']);
});
});

describe('sorting by source', () => {
const testCompletionA = createDummyItem(
{
Expand Down
132 changes: 82 additions & 50 deletions packages/jupyterlab-lsp/src/features/completion/model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,50 +59,6 @@ export class GenericCompleterModel<
return item as T;
}

setCompletionItems(newValue: T[]) {
super.setCompletionItems(newValue);

if (this.current && this.cursor) {
// set initial query to pre-filter items; in future we should use:
// https://github.com/jupyterlab/jupyterlab/issues/9763#issuecomment-1001603348

// note: start/end from cursor are not ideal because these get populated from fetch
// reply which will vary depending on what providers decide to return; we want the
// actual position in token, the same as passed in request to fetch. We can get it
// by searching for longest common prefix as seen below (or by counting characters).
// Maybe upstream should expose it directly?
const { start, end } = this.cursor;
const { text, line, column } = this.original!;

const queryRange = text.substring(start, end).trim();
const linePrefix = text.split('\n')[line].substring(0, column).trim();
let query = '';
for (let i = queryRange.length; i > 0; i--) {
if (queryRange.slice(0, i) == linePrefix.slice(-i)) {
query = linePrefix.slice(-i);
break;
}
}
if (!query) {
return;
}

let trimmedQuotes = false;
// special case for "Completes Paths In Strings" test case
if (query.startsWith('"') || query.startsWith("'")) {
query = query.substring(1);
trimmedQuotes = true;
}
if (query.endsWith('"') || query.endsWith("'")) {
query = query.substring(0, -1);
trimmedQuotes = true;
}
if (this.settings.preFilterMatches || trimmedQuotes) {
this.query = query;
}
}
}

private _markFragment(value: string): string {
return `<mark>${value}</mark>`;
}
Expand Down Expand Up @@ -138,7 +94,11 @@ export class GenericCompleterModel<
return super.createPatch(patch);
}

private _sortAndFilter(query: string, items: T[]): T[] {
resolveQuery(userQuery: string, _item: T) {
krassowski marked this conversation as resolved.
Show resolved Hide resolved
return userQuery;
}

private _sortAndFilter(userQuery: string, items: T[]): T[] {
let results: ICompletionMatch<T>[] = [];

for (let item of items) {
Expand All @@ -149,6 +109,8 @@ export class GenericCompleterModel<
let filterText: string | null = null;
let filterMatch: StringExt.IMatchResult | null = null;

const query = this.resolveQuery(userQuery, item);

let lowerCaseQuery = query.toLowerCase();

if (query) {
Expand Down Expand Up @@ -246,10 +208,6 @@ export namespace GenericCompleterModel {
* Whether perfect matches should be included (default = true)
*/
includePerfectMatches?: boolean;
/**
* Whether matches should be pre-filtered (default = true)
*/
preFilterMatches?: boolean;
/**
* Whether kernel completions should be shown first.
*/
Expand All @@ -258,7 +216,6 @@ export namespace GenericCompleterModel {
export const defaultOptions: IOptions = {
caseSensitive: true,
includePerfectMatches: true,
preFilterMatches: true,
kernelCompletionsFirst: false
};
}
Expand All @@ -267,13 +224,73 @@ type MaybeCompletionItem = Partial<CompletionItem> &
CompletionHandler.ICompletionItem;

export class LSPCompleterModel extends GenericCompleterModel<MaybeCompletionItem> {
public settings: LSPCompleterModel.IOptions;

constructor(settings: LSPCompleterModel.IOptions = {}) {
super();
this.settings = { ...LSPCompleterModel.defaultOptions, ...settings };
}

protected getFilterText(item: MaybeCompletionItem): string {
if (item.filterText) {
return item.filterText;
}
return super.getFilterText(item);
}

setCompletionItems(newValue: MaybeCompletionItem[]) {
super.setCompletionItems(newValue);
this._preFilterQuery = '';

if (this.current && this.cursor) {
// set initial query to pre-filter items; in future we should use:
// https://github.com/jupyterlab/jupyterlab/issues/9763#issuecomment-1001603348

// note: start/end from cursor are not ideal because these get populated from fetch
// reply which will vary depending on what providers decide to return; we want the
// actual position in token, the same as passed in request to fetch. We can get it
// by searching for longest common prefix as seen below (or by counting characters).
// Maybe upstream should expose it directly?
const { start, end } = this.cursor;
const { text, line, column } = this.original!;

const queryRange = text.substring(start, end).trim();
const linePrefix = text.split('\n')[line].substring(0, column).trim();
let query = '';
for (let i = queryRange.length; i > 0; i--) {
if (queryRange.slice(0, i) == linePrefix.slice(-i)) {
query = linePrefix.slice(-i);
break;
}
}
if (!query) {
return;
}

let trimmedQuotes = false;
// special case for "Completes Paths In Strings" test case
if (query.startsWith('"') || query.startsWith("'")) {
query = query.substring(1);
trimmedQuotes = true;
}
if (query.endsWith('"') || query.endsWith("'")) {
query = query.substring(0, -1);
trimmedQuotes = true;
}
if (this.settings.preFilterMatches || trimmedQuotes) {
this._preFilterQuery = query;
}
}
}

resolveQuery(userQuery: string, item: MaybeCompletionItem) {
krassowski marked this conversation as resolved.
Show resolved Hide resolved
return userQuery
? userQuery
: item.source === 'LSP'
? this._preFilterQuery
: '';
}

protected harmoniseItem(item: CompletionHandler.ICompletionItem) {
if ((item as any).self) {
const self = (item as any).self;
Expand Down Expand Up @@ -306,4 +323,19 @@ export class LSPCompleterModel extends GenericCompleterModel<MaybeCompletionItem
a.score - b.score
);
}

private _preFilterQuery: string = '';
}

export namespace LSPCompleterModel {
export interface IOptions extends GenericCompleterModel.IOptions {
/**
* Whether matches should be pre-filtered (default = true)
*/
preFilterMatches?: boolean;
}
export const defaultOptions: IOptions = {
...GenericCompleterModel.defaultOptions,
preFilterMatches: true
};
}