From 89a5ead9dd6e109ddf387656a628091f485523a8 Mon Sep 17 00:00:00 2001 From: krassowski Date: Sat, 4 Jan 2020 22:15:27 +0000 Subject: [PATCH 1/3] Implement a UX workaround for rope/pyls rename issue for the case of syntax errors in the source code. --- .../codemirror/features/diagnostics.ts | 43 ++++++++-- .../adapters/codemirror/features/rename.ts | 81 ++++++++++++++++++- .../src/adapters/jupyterlab/jl_adapter.ts | 11 ++- .../jupyterlab-lsp/src/command_manager.ts | 2 + .../src/virtual/editors/notebook.ts | 24 +++++- 5 files changed, 146 insertions(+), 15 deletions(-) diff --git a/packages/jupyterlab-lsp/src/adapters/codemirror/features/diagnostics.ts b/packages/jupyterlab-lsp/src/adapters/codemirror/features/diagnostics.ts index 8908512a7..e8bf5d036 100644 --- a/packages/jupyterlab-lsp/src/adapters/codemirror/features/diagnostics.ts +++ b/packages/jupyterlab-lsp/src/adapters/codemirror/features/diagnostics.ts @@ -1,7 +1,7 @@ import * as CodeMirror from 'codemirror'; import * as lsProtocol from 'vscode-languageserver-protocol'; import { PositionConverter } from '../../../converter'; -import { IVirtualPosition } from '../../../positioning'; +import { IEditorPosition, IVirtualPosition } from '../../../positioning'; import { diagnosticSeverityNames } from '../../../lsp'; import { DefaultMap } from '../../../utils'; import { CodeMirrorLSPFeature } from '../feature'; @@ -9,6 +9,19 @@ import { CodeMirrorLSPFeature } from '../feature'; // TODO: settings const default_severity = 2; +/** + * Diagnostic which is localized at a specific editor (cell) within a notebook + * (if used in the context of a FileEditor, then there is just a single editor) + */ +export interface IEditorDiagnostic { + diagnostic: lsProtocol.Diagnostic; + editor: CodeMirror.Editor; + range: { + start: IEditorPosition; + end: IEditorPosition; + }; +} + export class Diagnostics extends CodeMirrorLSPFeature { register(): void { this.connection_handlers.set( @@ -21,6 +34,14 @@ export class Diagnostics extends CodeMirrorLSPFeature { private unique_editor_ids: DefaultMap; private marked_diagnostics: Map = new Map(); + /** + * Allows access to the most recent diagnostics in context of the editor. + * + * One can use VirtualEditorForNotebook.find_cell_by_editor() to find + * the corresponding cell in notebook. + * Can be used to implement a Panel showing diagnostics list. + */ + public diagnostics_db: IEditorDiagnostic[]; protected collapse_overlapping_diagnostics( diagnostics: lsProtocol.Diagnostic[] @@ -74,6 +95,7 @@ export class Diagnostics extends CodeMirrorLSPFeature { public handleDiagnostic(response: lsProtocol.PublishDiagnosticsParams) { /* TODO: gutters */ try { + let diagnostics_db: IEditorDiagnostic[] = []; // Note: no deep equal for Sets or Maps in JS const markers_to_retain: Set = new Set(); @@ -146,6 +168,10 @@ export class Diagnostics extends CodeMirrorLSPFeature { let start_in_editor = document.transform_virtual_to_editor(start); let end_in_editor = document.transform_virtual_to_editor(end); + let range_in_editor = { + start: start_in_editor, + end: end_in_editor + }; // what a pity there is no hash in the standard library... // we could use this: https://stackoverflow.com/a/7616484 though it may not be worth it: // the stringified diagnostic objects are only about 100-200 JS characters anyway, @@ -166,12 +192,17 @@ export class Diagnostics extends CodeMirrorLSPFeature { // after the (inserted/removed) line - but such markers should not be invalidated, // i.e. the invalidation should be performed in the cell space, not in the notebook coordinate space, // thus we transform the coordinates and keep the cell id in the hash - range: { - start: start_in_editor, - end: end_in_editor - }, + range: range_in_editor, editor: this.unique_editor_ids.get(cm_editor) }); + for (let diagnostic of diagnostics) { + diagnostics_db.push({ + diagnostic, + editor: cm_editor, + range: range_in_editor + }); + } + markers_to_retain.add(diagnostic_hash); if (!this.marked_diagnostics.has(diagnostic_hash)) { @@ -201,6 +232,8 @@ export class Diagnostics extends CodeMirrorLSPFeature { // remove the markers which were not included in the new message this.remove_unused_diagnostic_markers(markers_to_retain); + + this.diagnostics_db = diagnostics_db; } catch (e) { console.warn(e); } diff --git a/packages/jupyterlab-lsp/src/adapters/codemirror/features/rename.ts b/packages/jupyterlab-lsp/src/adapters/codemirror/features/rename.ts index afbd6fffa..1e2a47172 100644 --- a/packages/jupyterlab-lsp/src/adapters/codemirror/features/rename.ts +++ b/packages/jupyterlab-lsp/src/adapters/codemirror/features/rename.ts @@ -5,16 +5,36 @@ import { IFeatureCommand } from '../feature'; import { InputDialog } from '@jupyterlab/apputils'; +import { Diagnostics } from './diagnostics'; +import { VirtualEditor } from '../../../virtual/editor'; +import { VirtualEditorForNotebook } from '../../../virtual/editors/notebook'; export class Rename extends CodeMirrorLSPFeature { static commands: Array = [ { id: 'rename-symbol', - execute: ({ connection, virtual_position, document, features }) => { + execute: ({ + editor, + connection, + virtual_position, + document, + features + }) => { let old_value = document.getTokenAt(virtual_position).string; let handle_failure = (error: any) => { - let feature = features.get('Rename') as Rename; - feature.status_message.set(`Rename failed: ${error}`, 5 * 1000); + let rename_feature = features.get('Rename') as Rename; + let diagnostics_feature = features.get('Diagnostics') as Diagnostics; + + let status = ux_workaround_for_rope_limitation( + error, + diagnostics_feature, + editor + ); + if (!status) { + status = `Rename failed: ${error}`; + } + + rename_feature.status_message.set(status, 5 * 1000); }; InputDialog.getText({ @@ -64,3 +84,58 @@ export class Rename extends CodeMirrorLSPFeature { .catch(console.warn); } } + +/** + * In #115 a with rename for Python (when using pyls) was identified: + * it was failing with an obscure message when the source code could + * not be parsed correctly by rope (due to a user's syntax error). + * + * This function detects such a condition using diagnostics feature + * and provides a nice error message to the user. + */ +function ux_workaround_for_rope_limitation( + error: any, + diagnostics_feature: Diagnostics, + editor: VirtualEditor +): string { + let has_index_error = false; + try { + has_index_error = error.message.includes('IndexError'); + } catch (e) { + return null; + } + if (!has_index_error) { + return null; + } + let dire_python_errors = (diagnostics_feature.diagnostics_db || []).filter( + diagnostic => + diagnostic.diagnostic.message.includes('invalid syntax') || + diagnostic.diagnostic.message.includes('SyntaxError') || + diagnostic.diagnostic.message.includes('IndentationError') + ); + + if (dire_python_errors.length === 0) { + return null; + } + + let dire_errors = [ + ...new Set( + dire_python_errors.map(diagnostic => { + let message = diagnostic.diagnostic.message; + let start = diagnostic.range.start; + if (editor.has_cells) { + let notebook_editor = editor as VirtualEditorForNotebook; + let { cell_id } = notebook_editor.find_cell_by_editor( + diagnostic.editor + ); + let cell_nr = cell_id + 1; + // TODO: should we show "code cell" numbers, or just cell number? + return `${message} in cell ${cell_nr} at line ${start.line}`; + } else { + return `${message} at line ${start.line}`; + } + }) + ) + ].join(', '); + return `Syntax error(s) prevent rename: ${dire_errors}`; +} diff --git a/packages/jupyterlab-lsp/src/adapters/jupyterlab/jl_adapter.ts b/packages/jupyterlab-lsp/src/adapters/jupyterlab/jl_adapter.ts index 930e288b9..c4aa8848d 100644 --- a/packages/jupyterlab-lsp/src/adapters/jupyterlab/jl_adapter.ts +++ b/packages/jupyterlab-lsp/src/adapters/jupyterlab/jl_adapter.ts @@ -390,12 +390,17 @@ export abstract class JupyterLabWidgetAdapter get_context_from_context_menu(): ICommandContext { let root_position = this.get_position_from_context_menu(); let document = this.virtual_editor.document_at_root_position(root_position); - let connection = this.connection_manager.connections.get(document.id_path); let virtual_position = this.virtual_editor.root_position_to_virtual_position( root_position ); - let features = this.get_features(document); - return { document, connection, virtual_position, root_position, features }; + return { + document, + connection: this.connection_manager.connections.get(document.id_path), + virtual_position, + root_position, + features: this.get_features(document), + editor: this.virtual_editor + }; } public create_tooltip( diff --git a/packages/jupyterlab-lsp/src/command_manager.ts b/packages/jupyterlab-lsp/src/command_manager.ts index e913af0fd..804ea637b 100644 --- a/packages/jupyterlab-lsp/src/command_manager.ts +++ b/packages/jupyterlab-lsp/src/command_manager.ts @@ -13,6 +13,7 @@ import { INotebookTracker } from '@jupyterlab/notebook'; import { VirtualDocument } from './virtual/document'; import { LSPConnection } from './connection'; import { IRootPosition, IVirtualPosition } from './positioning'; +import { VirtualEditor } from './virtual/editor'; export const file_editor_adapters: Map = new Map(); export const notebook_adapters: Map = new Map(); @@ -158,4 +159,5 @@ export interface ICommandContext { virtual_position: IVirtualPosition; root_position: IRootPosition; features: Map; + editor: VirtualEditor; } diff --git a/packages/jupyterlab-lsp/src/virtual/editors/notebook.ts b/packages/jupyterlab-lsp/src/virtual/editors/notebook.ts index 8a4f9047d..c1c34c370 100644 --- a/packages/jupyterlab-lsp/src/virtual/editors/notebook.ts +++ b/packages/jupyterlab-lsp/src/virtual/editors/notebook.ts @@ -38,10 +38,6 @@ class DocDispatcher implements CodeMirror.Doc { ); } - // getValue(seperator?: string): string { - // return this.virtual_editor.getValue(); - // } - getCursor(start?: string): CodeMirror.Position { let cell = this.virtual_editor.notebook.activeCell; let active_editor = cell.editor as CodeMirrorEditor; @@ -360,4 +356,24 @@ export class VirtualEditorForNotebook extends VirtualEditor { }); } } + + /** + * Find a cell in notebook which uses given CodeMirror editor. + * This function is O(n) - when looking up many cells + * using a hashmap based approach may be more efficient. + * @param cm_editor + */ + find_cell_by_editor(cm_editor: CodeMirror.Editor) { + let cells = this.notebook.widgets; + for (let i = 0; i < cells.length; i++) { + let cell = cells[i]; + let cell_editor = (cell.editor as CodeMirrorEditor).editor; + if (cell_editor === cm_editor) { + return { + cell_id: i, + cell: cell + }; + } + } + } } From 9cef36429e746a1de575dc9888642e2d38e53ddc Mon Sep 17 00:00:00 2001 From: krassowski Date: Sat, 4 Jan 2020 22:21:41 +0000 Subject: [PATCH 2/3] Fix docstring --- .../jupyterlab-lsp/src/adapters/codemirror/features/rename.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/jupyterlab-lsp/src/adapters/codemirror/features/rename.ts b/packages/jupyterlab-lsp/src/adapters/codemirror/features/rename.ts index 1e2a47172..0887ad0c4 100644 --- a/packages/jupyterlab-lsp/src/adapters/codemirror/features/rename.ts +++ b/packages/jupyterlab-lsp/src/adapters/codemirror/features/rename.ts @@ -86,8 +86,8 @@ export class Rename extends CodeMirrorLSPFeature { } /** - * In #115 a with rename for Python (when using pyls) was identified: - * it was failing with an obscure message when the source code could + * In #115 an issue with rename for Python (when using pyls) was identified: + * rename was failing with an obscure message when the source code could * not be parsed correctly by rope (due to a user's syntax error). * * This function detects such a condition using diagnostics feature From de7ac646d5bc182660d6a0bdb702af41d998a5f8 Mon Sep 17 00:00:00 2001 From: krassowski Date: Sat, 4 Jan 2020 23:50:54 +0000 Subject: [PATCH 3/3] Increase the timeout for rename error message, as this require user to read a longer text --- .../jupyterlab-lsp/src/adapters/codemirror/features/rename.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/jupyterlab-lsp/src/adapters/codemirror/features/rename.ts b/packages/jupyterlab-lsp/src/adapters/codemirror/features/rename.ts index 0887ad0c4..fdab6a393 100644 --- a/packages/jupyterlab-lsp/src/adapters/codemirror/features/rename.ts +++ b/packages/jupyterlab-lsp/src/adapters/codemirror/features/rename.ts @@ -34,7 +34,7 @@ export class Rename extends CodeMirrorLSPFeature { status = `Rename failed: ${error}`; } - rename_feature.status_message.set(status, 5 * 1000); + rename_feature.status_message.set(status, 7.5 * 1000); }; InputDialog.getText({