Skip to content

Commit 1552c1c

Browse files
krassowskikrassowski
authored
Merge pull request #833 from krassowski/completion/handle-missing-insertText-in-paths
Fix LSP completion transform of path-likes when `insertText` missing
2 parents d80ce36 + e4fe2f8 commit 1552c1c

File tree

3 files changed

+198
-92
lines changed

3 files changed

+198
-92
lines changed

‎CHANGELOG.md‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,8 @@
77
- implement settings UI using native JupyterLab 3.3 UI ([#778])
88
- bug fixes
99
- use correct websocket URL if configured as different from base URL ([#820], thanks @MikeSem)
10-
- clean up all completer styles when compelter feature is disabled ([#829]).
10+
- clean up all completer styles when completer feature is disabled ([#829]).
11+
- fix `undefined` being inserted for path-like completion items with no `insertText` ([#833])
1112
- refactoring:
1213
- move client capabilities to features ([#738])
1314
- documentation:
@@ -28,6 +29,7 @@
2829
[#820]: https://github.com/jupyter-lsp/jupyterlab-lsp/pull/820
2930
[#826]: https://github.com/jupyter-lsp/jupyterlab-lsp/pull/826
3031
[#829]: https://github.com/jupyter-lsp/jupyterlab-lsp/pull/829
32+
[#833]: https://github.com/jupyter-lsp/jupyterlab-lsp/pull/833
3133

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

‎packages/jupyterlab-lsp/src/features/completion/completion_handler.spec.ts‎

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
import { CodeEditor } from '@jupyterlab/codeeditor';
2+
import { expect } from 'chai';
3+
import type * as lsProtocol from 'vscode-languageserver-types';
4+
5+
import { BrowserConsole } from '../../virtual/console';
6+
7+
import { transformLSPCompletions } from './completion_handler';
8+
9+
describe('transformLSPCompletions', () => {
10+
const browserConsole = new BrowserConsole();
11+
12+
const transform = (
13+
token: CodeEditor.IToken,
14+
position: number,
15+
completions: lsProtocol.CompletionItem[]
16+
) => {
17+
return transformLSPCompletions(
18+
token,
19+
position,
20+
completions,
21+
(kind: string, match: lsProtocol.CompletionItem) => {
22+
return { kind, match };
23+
},
24+
browserConsole
25+
);
26+
};
27+
28+
it('Harmonizes `insertText` from `undefined` when adjusting path-like completions', () => {
29+
// `import { } from '@lumino/<TAB>'`
30+
const result = transform(
31+
{
32+
offset: 16,
33+
value: "'@lumino/'",
34+
type: 'string'
35+
},
36+
8,
37+
[
38+
{
39+
label: 'algorithm',
40+
kind: 19,
41+
sortText: '1',
42+
data: {
43+
entryNames: ['algorithm']
44+
}
45+
},
46+
{
47+
label: 'application',
48+
kind: 19,
49+
sortText: '1',
50+
data: {
51+
entryNames: ['application']
52+
}
53+
}
54+
]
55+
);
56+
expect(result.items.length).to.equal(2);
57+
// insert text should keep `'` as it was part of original token
58+
expect(result.items[0].match.insertText).to.equal("'@lumino/algorithm");
59+
// label should not include `'`
60+
expect(result.items[0].match.label).to.equal('@lumino/algorithm');
61+
expect(result.source.name).to.equal('LSP');
62+
});
63+
64+
it('Harmonizes `insertText` for paths', () => {
65+
// `'./Hov`
66+
const result = transform(
67+
{
68+
offset: 0,
69+
value: "'./Hov",
70+
type: 'string'
71+
},
72+
5,
73+
[
74+
{
75+
label: "Hover.ipynb'",
76+
kind: 17,
77+
sortText: "aHover.ipynb'",
78+
insertText: "Hover.ipynb'"
79+
}
80+
]
81+
);
82+
// insert text should keep `'` as it was part of original token
83+
expect(result.items[0].match.insertText).to.equal("'./Hover.ipynb'");
84+
// label should not include initial `'`
85+
expect(result.items[0].match.label).to.equal("./Hover.ipynb'");
86+
});
87+
});

‎packages/jupyterlab-lsp/src/features/completion/completion_handler.ts‎

Lines changed: 108 additions & 91 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import {
1212
KernelKind
1313
} from '@krassowski/completion-theme/lib/types';
1414
import { JSONArray, JSONObject } from '@lumino/coreutils';
15-
import * as lsProtocol from 'vscode-languageserver-types';
15+
import type * as lsProtocol from 'vscode-languageserver-types';
1616

1717
import { CodeCompletion as LSPCompletionSettings } from '../../_completion';
1818
import { LSPConnection } from '../../connection';
@@ -53,6 +53,99 @@ export interface ICompletionsReply
5353
items: IExtendedCompletionItem[];
5454
}
5555

56+
export function transformLSPCompletions<T>(
57+
token: CodeEditor.IToken,
58+
position_in_token: number,
59+
lspCompletionItems: lsProtocol.CompletionItem[],
60+
createCompletionItem: (kind: string, match: lsProtocol.CompletionItem) => T,
61+
console: ILSPLogConsole
62+
) {
63+
let prefix = token.value.slice(0, position_in_token + 1);
64+
let all_non_prefixed = true;
65+
let items: T[] = [];
66+
lspCompletionItems.forEach(match => {
67+
let kind = match.kind ? CompletionItemKind[match.kind] : '';
68+
69+
// Update prefix values
70+
let text = match.insertText ? match.insertText : match.label;
71+
72+
// declare prefix presence if needed and update it
73+
if (text.toLowerCase().startsWith(prefix.toLowerCase())) {
74+
all_non_prefixed = false;
75+
if (prefix !== token.value) {
76+
if (text.toLowerCase().startsWith(token.value.toLowerCase())) {
77+
// given a completion insert text "display_table" and two test cases:
78+
// disp<tab>data → display_table<cursor>data
79+
// disp<tab>lay → display_table<cursor>
80+
// we have to adjust the prefix for the latter (otherwise we would get display_table<cursor>lay),
81+
// as we are constrained NOT to replace after the prefix (which would be "disp" otherwise)
82+
prefix = token.value;
83+
}
84+
}
85+
}
86+
// add prefix if needed
87+
else if (token.type === 'string' && prefix.includes('/')) {
88+
// special case for path completion in strings, ensuring that:
89+
// '/Com<tab> → '/Completion.ipynb
90+
// when the returned insert text is `Completion.ipynb` (the token here is `'/Com`)
91+
// developed against pyls and pylsp server, may not work well in other cases
92+
const parts = prefix.split('/');
93+
if (
94+
text.toLowerCase().startsWith(parts[parts.length - 1].toLowerCase())
95+
) {
96+
let pathPrefix = parts.slice(0, -1).join('/') + '/';
97+
match.insertText = pathPrefix + text;
98+
// for label removing the prefix quote if present
99+
if (pathPrefix.startsWith("'") || pathPrefix.startsWith('"')) {
100+
pathPrefix = pathPrefix.substr(1);
101+
}
102+
match.label = pathPrefix + match.label;
103+
all_non_prefixed = false;
104+
}
105+
}
106+
107+
let completionItem = createCompletionItem(kind, match);
108+
109+
items.push(completionItem);
110+
});
111+
console.debug('Transformed');
112+
// required to make the repetitive trigger characters like :: or ::: work for R with R languageserver,
113+
// see https://github.com/jupyter-lsp/jupyterlab-lsp/issues/436
114+
let prefix_offset = token.value.length;
115+
// completion of dictionaries for Python with jedi-language-server was
116+
// causing an issue for dic['<tab>'] case; to avoid this let's make
117+
// sure that prefix.length >= prefix.offset
118+
if (all_non_prefixed && prefix_offset > prefix.length) {
119+
prefix_offset = prefix.length;
120+
}
121+
122+
let response = {
123+
// note in the ContextCompleter it was:
124+
// start: token.offset,
125+
// end: token.offset + token.value.length,
126+
// which does not work with "from statistics import <tab>" as the last token ends at "t" of "import",
127+
// so the completer would append "mean" as "from statistics importmean" (without space!);
128+
// (in such a case the typedCharacters is undefined as we are out of range)
129+
// a different workaround would be to prepend the token.value prefix:
130+
// text = token.value + text;
131+
// but it did not work for "from statistics <tab>" and lead to "from statisticsimport" (no space)
132+
start: token.offset + (all_non_prefixed ? prefix_offset : 0),
133+
end: token.offset + prefix.length,
134+
items: items,
135+
source: {
136+
name: 'LSP',
137+
priority: 2
138+
}
139+
};
140+
if (response.start > response.end) {
141+
console.warn(
142+
'Response contains start beyond end; this should not happen!',
143+
response
144+
);
145+
}
146+
return response;
147+
}
148+
56149
/**
57150
* A LSP connector for completion handlers.
58151
*/
@@ -376,97 +469,21 @@ export class LSPConnector
376469
)) || []) as lsProtocol.CompletionItem[];
377470

378471
this.console.debug('Transforming');
379-
let prefix = token.value.slice(0, position_in_token + 1);
380-
let all_non_prefixed = true;
381-
let items: IExtendedCompletionItem[] = [];
382-
lspCompletionItems.forEach(match => {
383-
let kind = match.kind ? CompletionItemKind[match.kind] : '';
384-
385-
// Update prefix values
386-
let text = match.insertText ? match.insertText : match.label;
387-
388-
// declare prefix presence if needed and update it
389-
if (text.toLowerCase().startsWith(prefix.toLowerCase())) {
390-
all_non_prefixed = false;
391-
if (prefix !== token.value) {
392-
if (text.toLowerCase().startsWith(token.value.toLowerCase())) {
393-
// given a completion insert text "display_table" and two test cases:
394-
// disp<tab>data → display_table<cursor>data
395-
// disp<tab>lay → display_table<cursor>
396-
// we have to adjust the prefix for the latter (otherwise we would get display_table<cursor>lay),
397-
// as we are constrained NOT to replace after the prefix (which would be "disp" otherwise)
398-
prefix = token.value;
399-
}
400-
}
401-
}
402-
// add prefix if needed
403-
else if (token.type === 'string' && prefix.includes('/')) {
404-
// special case for path completion in strings, ensuring that:
405-
// '/Com<tab> → '/Completion.ipynb
406-
// when the returned insert text is `Completion.ipynb` (the token here is `'/Com`)
407-
// developed against pyls and pylsp server, may not work well in other cases
408-
const parts = prefix.split('/');
409-
if (
410-
text.toLowerCase().startsWith(parts[parts.length - 1].toLowerCase())
411-
) {
412-
let pathPrefix = parts.slice(0, -1).join('/') + '/';
413-
match.insertText = pathPrefix + match.insertText;
414-
// for label removing the prefix quote if present
415-
if (pathPrefix.startsWith("'") || pathPrefix.startsWith('"')) {
416-
pathPrefix = pathPrefix.substr(1);
417-
}
418-
match.label = pathPrefix + match.label;
419-
all_non_prefixed = false;
420-
}
421-
}
422-
423-
let completionItem = new LazyCompletionItem(
424-
kind,
425-
this.icon_for(kind),
426-
match,
427-
this,
428-
document.uri
429-
);
430472

431-
items.push(completionItem);
432-
});
433-
this.console.debug('Transformed');
434-
// required to make the repetitive trigger characters like :: or ::: work for R with R languageserver,
435-
// see https://github.com/jupyter-lsp/jupyterlab-lsp/issues/436
436-
let prefix_offset = token.value.length;
437-
// completion of dictionaries for Python with jedi-language-server was
438-
// causing an issue for dic['<tab>'] case; to avoid this let's make
439-
// sure that prefix.length >= prefix.offset
440-
if (all_non_prefixed && prefix_offset > prefix.length) {
441-
prefix_offset = prefix.length;
442-
}
443-
444-
let response = {
445-
// note in the ContextCompleter it was:
446-
// start: token.offset,
447-
// end: token.offset + token.value.length,
448-
// which does not work with "from statistics import <tab>" as the last token ends at "t" of "import",
449-
// so the completer would append "mean" as "from statistics importmean" (without space!);
450-
// (in such a case the typedCharacters is undefined as we are out of range)
451-
// a different workaround would be to prepend the token.value prefix:
452-
// text = token.value + text;
453-
// but it did not work for "from statistics <tab>" and lead to "from statisticsimport" (no space)
454-
start: token.offset + (all_non_prefixed ? prefix_offset : 0),
455-
end: token.offset + prefix.length,
456-
items: items,
457-
source: {
458-
name: 'LSP',
459-
priority: 2
460-
}
461-
};
462-
if (response.start > response.end) {
463-
console.warn(
464-
'Response contains start beyond end; this should not happen!',
465-
response
466-
);
467-
}
468-
469-
return response;
473+
return transformLSPCompletions(
474+
token,
475+
position_in_token,
476+
lspCompletionItems,
477+
(kind, match) =>
478+
new LazyCompletionItem(
479+
kind,
480+
this.icon_for(kind),
481+
match,
482+
this,
483+
document.uri
484+
),
485+
this.console
486+
);
470487
}
471488

472489
protected icon_for(type: string): LabIcon {

0 commit comments

Comments
 (0)