Skip to content

Commit

Permalink
Fix various bugs in the completer feature, closes #30
Browse files Browse the repository at this point in the history 
And add some test cases from code comments
  • Loading branch information
@krassowski
krassowski committed Jan 19, 2020
1 parent 211716b commit bf9d4b9
Show file tree
Hide file tree
Showing 4 changed files with 176 additions and 14 deletions.
55 changes: 52 additions & 3 deletions atest/05_Features/Completion.robot
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,54 @@ Autocompletes If Only One Option
User Can Select Lowercase After Starting Uppercase
[Tags] language:python
Setup Notebook Python Completion.ipynb
Enter Cell Editor 4 line=1
# `from time import Tim<tab>` → `from time import time`
Enter Cell Editor 5 line=1
Trigger Completer
Completer Should Suggest time
Press Keys None ENTER
Wait Until Keyword Succeeds 40x 0.5s Cell Editor Should Equal 4 from time import time
Wait Until Keyword Succeeds 40x 0.5s Cell Editor Should Equal 5 from time import time
[Teardown] Clean Up After Working With File Completion.ipynb

Mid Token Completions Do Not Overwrite
[Tags] language:python
Setup Notebook Python Completion.ipynb
# `disp<tab>data` → `display_table<cursor>data`
Place Cursor In Cell Editor At 9 line=1 character=4
Capture Page Screenshot 01-cursor-placed.png
Wait Until Fully Initialized
Press Keys None TAB
Capture Page Screenshot 02-completed.png
Wait Until Keyword Succeeds 40x 0.5s Cell Editor Should Equal 9 display_tabledata
# `disp<tab>lay` → `display_table<cursor>`
Place Cursor In Cell Editor At 11 line=1 character=4
Press Keys None TAB
Wait Until Keyword Succeeds 40x 0.5s Cell Editor Should Equal 11 display_table
[Teardown] Clean Up After Working With File Completion.ipynb

Completion Works For Tokens Separated By Space
[Tags] language:python
Setup Notebook Python Completion.ipynb
# `from statistics <tab>` → `from statistics import<cursor>`
Enter Cell Editor 13 line=1
Wait Until Fully Initialized
Trigger Completer
Completer Should Suggest import
Press Keys None ENTER
Wait Until Keyword Succeeds 40x 0.5s Cell Editor Should Equal 13 from statistics import
[Teardown] Clean Up After Working With File Completion.ipynb

Kernel And LSP Completions Merge Prefix Conflicts Are Resolved
[Documentation] Reconciliate Python kernel returning prefixed completions and LSP (pyls) not-prefixed ones
[Tags] language:python
# For more details see: https://github.com/krassowski/jupyterlab-lsp/issues/30#issuecomment-576003987
# `import os.pat<tab>` → `import os.pathsep`
Setup Notebook Python Completion.ipynb
Enter Cell Editor 15 line=1
Wait Until Fully Initialized
Trigger Completer
Completer Should Suggest pathsep
Select Completer Suggestion pathsep
Wait Until Keyword Succeeds 40x 0.5s Cell Editor Should Equal 15 import os.pathsep
[Teardown] Clean Up After Working With File Completion.ipynb

Triggers Completer On Dot
Expand All @@ -79,14 +122,20 @@ Triggers Completer On Dot
*** Keywords ***
Get Cell Editor Content
[Arguments] ${cell_nr}
${content} Execute JavaScript return document.querySelector('.jp-CodeCell:nth-child(${cell_nr}) .CodeMirror').CodeMirror.getValue()
${content} Execute JavaScript return document.querySelector('.jp-Cell:nth-child(${cell_nr}) .CodeMirror').CodeMirror.getValue()
[Return] ${content}

Cell Editor Should Equal
[Arguments] ${cell} ${value}
${content} = Get Cell Editor Content ${cell}
Should Be Equal ${content} ${value}

Select Completer Suggestion
[Arguments] ${text}
${suggestion} = Set Variable css:.jp-Completer-item[data-value="${text}"]
Mouse Over ${suggestion}
Click Element ${suggestion} code

Completer Should Suggest
[Arguments] ${text}
Page Should Contain Element ${COMPLETER_BOX} .jp-Completer-item[data-value="${text}"]
Expand Down
9 changes: 7 additions & 2 deletions atest/Keywords.robot
View file
Original file line number Diff line number Diff line change
Expand Up @@ -198,8 +198,13 @@ Gently Reset Workspace

Enter Cell Editor
[Arguments] ${cell_nr} ${line}=1
Click Element css:.jp-CodeCell:nth-child(${cell_nr}) .CodeMirror-line:nth-child(${line})
Wait Until Page Contains Element css:.jp-CodeCell:nth-child(${cell_nr}) .CodeMirror-focused
Click Element css:.jp-Cell:nth-child(${cell_nr}) .CodeMirror-line:nth-child(${line})
Wait Until Page Contains Element css:.jp-Cell:nth-child(${cell_nr}) .CodeMirror-focused

Place Cursor In Cell Editor At
[Arguments] ${cell_nr} ${line} ${character}
Enter Cell Editor ${cell_nr} ${line}
Execute JavaScript return document.querySelector('.jp-Cell:nth-child(${cell_nr}) .CodeMirror').CodeMirror.setCursor({line: ${line} - 1, ch: ${character}})

Wait Until Fully Initialized
Wait Until Element Contains ${STATUSBAR} Fully initialized timeout=35s
88 changes: 88 additions & 0 deletions atest/examples/Completion.ipynb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,13 @@
"list."
]
},
{
"cell_type": "markdown",
"metadata": {},
"source": [
"`from time import Tim<tab>` → `from time import time`"
]
},
{
"cell_type": "code",
"execution_count": null,
Expand All @@ -36,6 +43,87 @@
"source": [
"from time import Tim"
]
},
{
"cell_type": "markdown",
"metadata": {},
"source": [
"Setup (not a test case)"
]
},
{
"cell_type": "code",
"execution_count": null,
"metadata": {},
"outputs": [],
"source": [
"def display_table():\n",
" pass"
]
},
{
"cell_type": "markdown",
"metadata": {},
"source": [
"`disp<tab>data` → `display_table<cursor>data`"
]
},
{
"cell_type": "code",
"execution_count": null,
"metadata": {},
"outputs": [],
"source": [
"dispdata"
]
},
{
"cell_type": "markdown",
"metadata": {},
"source": [
"`disp<tab>lay` → `display_table<cursor>`"
]
},
{
"cell_type": "code",
"execution_count": null,
"metadata": {},
"outputs": [],
"source": [
"display"
]
},
{
"cell_type": "markdown",
"metadata": {},
"source": [
"`from statistics <tab>` → `from statistics import<cursor>`"
]
},
{
"cell_type": "code",
"execution_count": null,
"metadata": {},
"outputs": [],
"source": [
"from statistics "
]
},
{
"cell_type": "markdown",
"metadata": {},
"source": [
"`import os.pat<tab>` → select `pathsep` → `import os.pathsep` (tests whether kernel prefixes are removed)"
]
},
{
"cell_type": "code",
"execution_count": null,
"metadata": {},
"outputs": [],
"source": [
"import os.pat"
]
}
],
"metadata": {
Expand Down
38 changes: 29 additions & 9 deletions packages/jupyterlab-lsp/src/adapters/jupyterlab/components/completion.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ export class LSPConnector extends DataConnector<
const start = editor.getPositionAt(token.offset);
const end = editor.getPositionAt(token.offset + token.value.length);

let position_in_token = cursor.column - start.column - 1;
const typed_character = token.value[cursor.column - start.column - 1];

let start_in_root = this.transform_from_editor_to_root(start);
Expand Down Expand Up @@ -150,7 +151,8 @@ export class LSPConnector extends DataConnector<
virtual_start,
virtual_end,
virtual_cursor,
document
document,
position_in_token
)
]).then(([kernel, lsp]) =>
this.merge_replies(kernel, lsp, this._editor)
Expand All @@ -163,7 +165,8 @@ export class LSPConnector extends DataConnector<
virtual_start,
virtual_end,
virtual_cursor,
document
document,
position_in_token
).catch(e => {
console.warn('LSP: hint failed', e);
return this.fallback_connector.fetch(request);
Expand All @@ -180,7 +183,8 @@ export class LSPConnector extends DataConnector<
start: IVirtualPosition,
end: IVirtualPosition,
cursor: IVirtualPosition,
document: VirtualDocument
document: VirtualDocument,
position_in_token: number
): Promise<CompletionHandler.IReply> {
let connection = this._connections.get(document.id_path);

Expand All @@ -190,7 +194,7 @@ export class LSPConnector extends DataConnector<
// to the matches...
// Suggested in https://github.com/jupyterlab/jupyterlab/issues/7044, TODO PR

console.log(token);
console.log('[LSP][Completer] Token:', token);

let completion_items: lsProtocol.CompletionItem[] = [];
await connection
Expand All @@ -208,6 +212,8 @@ export class LSPConnector extends DataConnector<
completion_items = items || [];
});

let prefix = token.value.slice(0, position_in_token + 1);

let matches: Array<string> = [];
const types: Array<IItemType> = [];
let all_non_prefixed = true;
Expand All @@ -219,10 +225,22 @@ export class LSPConnector extends DataConnector<
// kind: 3
// label: "mean(data)"
// sortText: "amean"

// TODO: add support for match.textEdit
let text = match.insertText ? match.insertText : match.label;

if (text.toLowerCase().startsWith(token.value.toLowerCase())) {
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;
}
}
}

matches.push(text);
Expand All @@ -243,7 +261,7 @@ export class LSPConnector extends DataConnector<
// 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 ? 1 : 0),
end: token.offset + token.value.length,
end: token.offset + prefix.length,
matches: matches,
metadata: {
_jupyter_types_experimental: types
Expand All @@ -266,7 +284,7 @@ export class LSPConnector extends DataConnector<
} else if (lsp.matches.length === 0) {
return kernel;
}
console.log('merging LSP and kernel completions:', lsp, kernel);
console.log('[LSP][Completer] Merging completions:', lsp, kernel);

// Populate the result with a copy of the lsp matches.
const matches = lsp.matches.slice();
Expand All @@ -285,8 +303,10 @@ export class LSPConnector extends DataConnector<
if (lsp.start > kernel.start) {
const cursor = editor.getCursorPosition();
const line = editor.getLine(cursor.line);
prefix = line.substring(cursor.column - 1, cursor.column);
console.log('will remove prefix from kernel response:', prefix);
prefix = line.substring(kernel.start, lsp.start);
console.log('[LSP][Completer] Removing kernel prefix: ', prefix);
} else if (lsp.start < kernel.start) {
console.warn('[LSP][Completer] Kernel start > LSP start');
}

let remove_prefix = (value: string) => {
Expand Down

0 comments on commit bf9d4b9

Please sign in to comment.