Skip to content

Commit

Permalink
Do not pre-filter completions from kernel (#1022)
Browse files Browse the repository at this point in the history 
* Address jest deprecation warning

* Only pre-filter LSP completions, not the kernel completions

* Apply suggestions from code review

Make newly added `resolveQuery` protected
  • Loading branch information
@krassowski
krassowski committed Nov 26, 2023
1 parent f379b6d commit ea1b220
Show file tree
Hide file tree
Showing 3 changed files with 129 additions and 53 deletions.
3 changes: 1 addition & 2 deletions packages/jupyterlab-lsp/jest.config.js
View file
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
View file
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
View file
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[] {
protected resolveQuery(userQuery: string, _item: T) {
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;
}
}
}

protected resolveQuery(userQuery: string, item: MaybeCompletionItem) {
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
};
}

0 comments on commit ea1b220

Please sign in to comment.