Skip to content

Commit

Permalink
Merge pull request #833 from krassowski/completion/handle-missing-ins…
Browse files Browse the repository at this point in the history 
…ertText-in-paths

Fix LSP completion transform of path-likes when `insertText` missing
  • Loading branch information
@krassowski
krassowski authored Aug 23, 2022
2 parents d80ce36 + e4fe2f8 commit 1552c1c
Show file tree
Hide file tree
Showing 3 changed files with 198 additions and 92 deletions.
4 changes: 3 additions & 1 deletion CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@
- implement settings UI using native JupyterLab 3.3 UI ([#778])
- bug fixes
- use correct websocket URL if configured as different from base URL ([#820], thanks @MikeSem)
- clean up all completer styles when compelter feature is disabled ([#829]).
- clean up all completer styles when completer feature is disabled ([#829]).
- fix `undefined` being inserted for path-like completion items with no `insertText` ([#833])
- refactoring:
- move client capabilities to features ([#738])
- documentation:
Expand All @@ -28,6 +29,7 @@
[#820]: https://github.com/jupyter-lsp/jupyterlab-lsp/pull/820
[#826]: https://github.com/jupyter-lsp/jupyterlab-lsp/pull/826
[#829]: https://github.com/jupyter-lsp/jupyterlab-lsp/pull/829
[#833]: https://github.com/jupyter-lsp/jupyterlab-lsp/pull/833

### `@krassowski/jupyterlab-lsp 3.10.1` (2022-03-21)

Expand Down
87 changes: 87 additions & 0 deletions packages/jupyterlab-lsp/src/features/completion/completion_handler.spec.ts
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
import { CodeEditor } from '@jupyterlab/codeeditor';
import { expect } from 'chai';
import type * as lsProtocol from 'vscode-languageserver-types';

import { BrowserConsole } from '../../virtual/console';

import { transformLSPCompletions } from './completion_handler';

describe('transformLSPCompletions', () => {
const browserConsole = new BrowserConsole();

const transform = (
token: CodeEditor.IToken,
position: number,
completions: lsProtocol.CompletionItem[]
) => {
return transformLSPCompletions(
token,
position,
completions,
(kind: string, match: lsProtocol.CompletionItem) => {
return { kind, match };
},
browserConsole
);
};

it('Harmonizes `insertText` from `undefined` when adjusting path-like completions', () => {
// `import { } from '@lumino/<TAB>'`
const result = transform(
{
offset: 16,
value: "'@lumino/'",
type: 'string'
},
8,
[
{
label: 'algorithm',
kind: 19,
sortText: '1',
data: {
entryNames: ['algorithm']
}
},
{
label: 'application',
kind: 19,
sortText: '1',
data: {
entryNames: ['application']
}
}
]
);
expect(result.items.length).to.equal(2);
// insert text should keep `'` as it was part of original token
expect(result.items[0].match.insertText).to.equal("'@lumino/algorithm");
// label should not include `'`
expect(result.items[0].match.label).to.equal('@lumino/algorithm');
expect(result.source.name).to.equal('LSP');
});

it('Harmonizes `insertText` for paths', () => {
// `'./Hov`
const result = transform(
{
offset: 0,
value: "'./Hov",
type: 'string'
},
5,
[
{
label: "Hover.ipynb'",
kind: 17,
sortText: "aHover.ipynb'",
insertText: "Hover.ipynb'"
}
]
);
// insert text should keep `'` as it was part of original token
expect(result.items[0].match.insertText).to.equal("'./Hover.ipynb'");
// label should not include initial `'`
expect(result.items[0].match.label).to.equal("./Hover.ipynb'");
});
});
199 changes: 108 additions & 91 deletions packages/jupyterlab-lsp/src/features/completion/completion_handler.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {
KernelKind
} from '@krassowski/completion-theme/lib/types';
import { JSONArray, JSONObject } from '@lumino/coreutils';
import * as lsProtocol from 'vscode-languageserver-types';
import type * as lsProtocol from 'vscode-languageserver-types';

import { CodeCompletion as LSPCompletionSettings } from '../../_completion';
import { LSPConnection } from '../../connection';
Expand Down Expand Up @@ -53,6 +53,99 @@ export interface ICompletionsReply
items: IExtendedCompletionItem[];
}

export function transformLSPCompletions<T>(
token: CodeEditor.IToken,
position_in_token: number,
lspCompletionItems: lsProtocol.CompletionItem[],
createCompletionItem: (kind: string, match: lsProtocol.CompletionItem) => T,
console: ILSPLogConsole
) {
let prefix = token.value.slice(0, position_in_token + 1);
let all_non_prefixed = true;
let items: T[] = [];
lspCompletionItems.forEach(match => {
let kind = match.kind ? CompletionItemKind[match.kind] : '';

// Update prefix values
let text = match.insertText ? match.insertText : match.label;

// declare prefix presence if needed and update it
if (text.toLowerCase().startsWith(prefix.toLowerCase())) {
all_non_prefixed = false;
if (prefix !== token.value) {
if (text.toLowerCase().startsWith(token.value.toLowerCase())) {
// given a completion insert text "display_table" and two test cases:
// disp<tab>data → display_table<cursor>data
// disp<tab>lay → display_table<cursor>
// we have to adjust the prefix for the latter (otherwise we would get display_table<cursor>lay),
// as we are constrained NOT to replace after the prefix (which would be "disp" otherwise)
prefix = token.value;
}
}
}
// add prefix if needed
else if (token.type === 'string' && prefix.includes('/')) {
// special case for path completion in strings, ensuring that:
// '/Com<tab> → '/Completion.ipynb
// when the returned insert text is `Completion.ipynb` (the token here is `'/Com`)
// developed against pyls and pylsp server, may not work well in other cases
const parts = prefix.split('/');
if (
text.toLowerCase().startsWith(parts[parts.length - 1].toLowerCase())
) {
let pathPrefix = parts.slice(0, -1).join('/') + '/';
match.insertText = pathPrefix + text;
// for label removing the prefix quote if present
if (pathPrefix.startsWith("'") || pathPrefix.startsWith('"')) {
pathPrefix = pathPrefix.substr(1);
}
match.label = pathPrefix + match.label;
all_non_prefixed = false;
}
}

let completionItem = createCompletionItem(kind, match);

items.push(completionItem);
});
console.debug('Transformed');
// required to make the repetitive trigger characters like :: or ::: work for R with R languageserver,
// see https://github.com/jupyter-lsp/jupyterlab-lsp/issues/436
let prefix_offset = token.value.length;
// completion of dictionaries for Python with jedi-language-server was
// causing an issue for dic['<tab>'] case; to avoid this let's make
// sure that prefix.length >= prefix.offset
if (all_non_prefixed && prefix_offset > prefix.length) {
prefix_offset = prefix.length;
}

let response = {
// note in the ContextCompleter it was:
// start: token.offset,
// end: token.offset + token.value.length,
// which does not work with "from statistics import <tab>" as the last token ends at "t" of "import",
// so the completer would append "mean" as "from statistics importmean" (without space!);
// (in such a case the typedCharacters is undefined as we are out of range)
// a different workaround would be to prepend the token.value prefix:
// text = token.value + text;
// but it did not work for "from statistics <tab>" and lead to "from statisticsimport" (no space)
start: token.offset + (all_non_prefixed ? prefix_offset : 0),
end: token.offset + prefix.length,
items: items,
source: {
name: 'LSP',
priority: 2
}
};
if (response.start > response.end) {
console.warn(
'Response contains start beyond end; this should not happen!',
response
);
}
return response;
}

/**
* A LSP connector for completion handlers.
*/
Expand Down Expand Up @@ -376,97 +469,21 @@ export class LSPConnector
)) || []) as lsProtocol.CompletionItem[];

this.console.debug('Transforming');
let prefix = token.value.slice(0, position_in_token + 1);
let all_non_prefixed = true;
let items: IExtendedCompletionItem[] = [];
lspCompletionItems.forEach(match => {
let kind = match.kind ? CompletionItemKind[match.kind] : '';

// Update prefix values
let text = match.insertText ? match.insertText : match.label;

// declare prefix presence if needed and update it
if (text.toLowerCase().startsWith(prefix.toLowerCase())) {
all_non_prefixed = false;
if (prefix !== token.value) {
if (text.toLowerCase().startsWith(token.value.toLowerCase())) {
// given a completion insert text "display_table" and two test cases:
// disp<tab>data → display_table<cursor>data
// disp<tab>lay → display_table<cursor>
// we have to adjust the prefix for the latter (otherwise we would get display_table<cursor>lay),
// as we are constrained NOT to replace after the prefix (which would be "disp" otherwise)
prefix = token.value;
}
}
}
// add prefix if needed
else if (token.type === 'string' && prefix.includes('/')) {
// special case for path completion in strings, ensuring that:
// '/Com<tab> → '/Completion.ipynb
// when the returned insert text is `Completion.ipynb` (the token here is `'/Com`)
// developed against pyls and pylsp server, may not work well in other cases
const parts = prefix.split('/');
if (
text.toLowerCase().startsWith(parts[parts.length - 1].toLowerCase())
) {
let pathPrefix = parts.slice(0, -1).join('/') + '/';
match.insertText = pathPrefix + match.insertText;
// for label removing the prefix quote if present
if (pathPrefix.startsWith("'") || pathPrefix.startsWith('"')) {
pathPrefix = pathPrefix.substr(1);
}
match.label = pathPrefix + match.label;
all_non_prefixed = false;
}
}

let completionItem = new LazyCompletionItem(
kind,
this.icon_for(kind),
match,
this,
document.uri
);

items.push(completionItem);
});
this.console.debug('Transformed');
// required to make the repetitive trigger characters like :: or ::: work for R with R languageserver,
// see https://github.com/jupyter-lsp/jupyterlab-lsp/issues/436
let prefix_offset = token.value.length;
// completion of dictionaries for Python with jedi-language-server was
// causing an issue for dic['<tab>'] case; to avoid this let's make
// sure that prefix.length >= prefix.offset
if (all_non_prefixed && prefix_offset > prefix.length) {
prefix_offset = prefix.length;
}

let response = {
// note in the ContextCompleter it was:
// start: token.offset,
// end: token.offset + token.value.length,
// which does not work with "from statistics import <tab>" as the last token ends at "t" of "import",
// so the completer would append "mean" as "from statistics importmean" (without space!);
// (in such a case the typedCharacters is undefined as we are out of range)
// a different workaround would be to prepend the token.value prefix:
// text = token.value + text;
// but it did not work for "from statistics <tab>" and lead to "from statisticsimport" (no space)
start: token.offset + (all_non_prefixed ? prefix_offset : 0),
end: token.offset + prefix.length,
items: items,
source: {
name: 'LSP',
priority: 2
}
};
if (response.start > response.end) {
console.warn(
'Response contains start beyond end; this should not happen!',
response
);
}

return response;
return transformLSPCompletions(
token,
position_in_token,
lspCompletionItems,
(kind, match) =>
new LazyCompletionItem(
kind,
this.icon_for(kind),
match,
this,
document.uri
),
this.console
);
}

protected icon_for(type: string): LabIcon {
Expand Down

0 comments on commit 1552c1c

Please sign in to comment.