diff --git a/CHANGELOG.md b/CHANGELOG.md index 723a0b131..c9370e7b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,9 @@ - cleans up documents, handlers, events, and signals more aggressively ([#165][]) - ignores malformed diagnostic ranges, enabling markdown support ([#165][]) - passes tests on Python 3.8 on Windows ([#165][]) + - improves support for rpy2 magic cells with parameters ( + [#206](https://github.com/krassowski/jupyterlab-lsp/pull/206) + ) - bug fixes diff --git a/packages/jupyterlab-lsp/src/extractors/defaults.spec.ts b/packages/jupyterlab-lsp/src/extractors/defaults.spec.ts index e2ba07a51..638ce3c40 100644 --- a/packages/jupyterlab-lsp/src/extractors/defaults.spec.ts +++ b/packages/jupyterlab-lsp/src/extractors/defaults.spec.ts @@ -17,15 +17,21 @@ describe('Default extractors', () => { }); } - function get_the_only_virtual( + function get_the_only_pair( foreign_document_map: Map ) { expect(foreign_document_map.size).to.equal(1); - let { virtual_document } = foreign_document_map.get( - foreign_document_map.keys().next().value - ); + let range = foreign_document_map.keys().next().value; + let { virtual_document } = foreign_document_map.get(range); + return { range, virtual_document }; + } + + function get_the_only_virtual( + foreign_document_map: Map + ) { + let { virtual_document } = get_the_only_pair(foreign_document_map); return virtual_document; } @@ -76,7 +82,9 @@ describe('Default extractors', () => { it('parses multiple inputs (into dummy data frames)', () => { let code = wrap_in_python_lines('%R -i df -i x ggplot(df)'); - let r_document = get_the_only_virtual(extract(code).foreign_document_map); + let { virtual_document: r_document } = get_the_only_pair( + extract(code).foreign_document_map + ); expect(r_document.value).to.equal( 'df <- data.frame(); x <- data.frame(); ggplot(df)\n' ); diff --git a/packages/jupyterlab-lsp/src/extractors/defaults.ts b/packages/jupyterlab-lsp/src/extractors/defaults.ts index 6787fa559..b6832c02b 100644 --- a/packages/jupyterlab-lsp/src/extractors/defaults.ts +++ b/packages/jupyterlab-lsp/src/extractors/defaults.ts @@ -6,20 +6,26 @@ import { RPY2_MAX_ARGS } from '../magics/rpy2'; -function rpy2_replacer(match: string, ...args: string[]) { +function rpy2_code_extractor(match: string, ...args: string[]) { let r = extract_r_args(args, -3); - // define dummy input variables using empty data frames - let inputs = r.inputs.map(i => i + ' <- data.frame();').join(' '); let code: string; if (r.rest == null) { code = ''; } else { code = r.rest.startsWith(' ') ? r.rest.slice(1) : r.rest; } + return code; +} + +function rpy2_args(match: string, ...args: string[]) { + let r = extract_r_args(args, -3); + // define dummy input variables using empty data frames + let inputs = r.inputs.map(i => i + ' <- data.frame();').join(' '); + let code = rpy2_code_extractor(match, ...args); if (inputs !== '' && code) { inputs += ' '; } - return `${inputs}${code}`; + return inputs; } // TODO: make the regex code extractors configurable @@ -32,14 +38,16 @@ export let foreign_code_extractors: IForeignCodeExtractorsRegistry = { new RegExpForeignCodeExtractor({ language: 'r', pattern: '^%%R' + rpy2_args_pattern(RPY2_MAX_ARGS) + '\n([^]*)', - extract_to_foreign: rpy2_replacer, + extract_to_foreign: rpy2_code_extractor, + extract_arguments: rpy2_args, is_standalone: false, file_extension: 'R' }), new RegExpForeignCodeExtractor({ language: 'r', pattern: '(?:^|\n)%R' + rpy2_args_pattern(RPY2_MAX_ARGS) + '( .*)?\n?', - extract_to_foreign: rpy2_replacer, + extract_to_foreign: rpy2_code_extractor, + extract_arguments: rpy2_args, is_standalone: false, file_extension: 'R' }), diff --git a/packages/jupyterlab-lsp/src/extractors/regexp.ts b/packages/jupyterlab-lsp/src/extractors/regexp.ts index 0fabb10eb..1351c504e 100644 --- a/packages/jupyterlab-lsp/src/extractors/regexp.ts +++ b/packages/jupyterlab-lsp/src/extractors/regexp.ts @@ -1,6 +1,7 @@ import { IExtractedCode, IForeignCodeExtractor } from './types'; import { position_at_offset } from '../positioning'; import { replacer } from '../magics/overrides'; +import { CodeEditor } from '@jupyterlab/codeeditor'; export class RegExpForeignCodeExtractor implements IForeignCodeExtractor { options: RegExpForeignCodeExtractor.IOptions; @@ -38,19 +39,29 @@ export class RegExpForeignCodeExtractor implements IForeignCodeExtractor { while (match !== null) { let matched_string = match[0]; + let position_shift: CodeEditor.IPosition = null; let foreign_code_fragment = matched_string.replace( this.expression, // @ts-ignore this.options.extract_to_foreign ); + let prefix = ''; + if (typeof this.options.extract_arguments !== 'undefined') { + prefix = matched_string.replace( + this.expression, + // @ts-ignore + this.options.extract_arguments + ); + position_shift = position_at_offset(prefix.length, prefix.split('\n')); + } // NOTE: // match.index + matched_string.length === this.sticky_expression.lastIndex - let end = this.global_expression.lastIndex; + let end_index = this.global_expression.lastIndex; if (this.options.keep_in_host || this.options.keep_in_host == null) { - host_code_fragment = code.substring(started_from, end); + host_code_fragment = code.substring(started_from, end_index); } else { if (started_from === match.index) { host_code_fragment = ''; @@ -62,15 +73,19 @@ export class RegExpForeignCodeExtractor implements IForeignCodeExtractor { // TODO: this could be slightly optimized (start at start) by using the match[n], // where n is the group to be used; while this reduces the flexibility of extract_to_foreign, // it might be better to enforce such strict requirement - let start = match.index + matched_string.indexOf(foreign_code_fragment); + let start_offset = + match.index + matched_string.indexOf(foreign_code_fragment); + let start = position_at_offset(start_offset, lines); + let end = position_at_offset( + start_offset + foreign_code_fragment.length, + lines + ); extracts.push({ host_code: host_code_fragment, - foreign_code: foreign_code_fragment, - range: { - start: position_at_offset(start, lines), - end: position_at_offset(start + foreign_code_fragment.length, lines) - } + foreign_code: prefix + foreign_code_fragment, + range: { start, end }, + virtual_shift: position_shift }); started_from = this.global_expression.lastIndex; @@ -82,7 +97,8 @@ export class RegExpForeignCodeExtractor implements IForeignCodeExtractor { extracts.push({ host_code: final_host_code_fragment, foreign_code: null, - range: null + range: null, + virtual_shift: null }); } @@ -111,7 +127,12 @@ namespace RegExpForeignCodeExtractor { */ extract_to_foreign: string | replacer; /** - * String boolean if everything (true, default) or nothing (false) should be kept in the host document. + * If arguments from the cell or line magic are to be extracted and prepended before the extracted code, + * set extract_arguments to a replacer function taking the code and returning the string to be prepended. + */ + extract_arguments?: replacer; + /** + * Boolean if everything (true, default) or nothing (false) should be kept in the host document. * * For the R example this should be empty if we wish to ignore the cell, * but usually a better option is to retain the foreign code and use language diff --git a/packages/jupyterlab-lsp/src/extractors/types.ts b/packages/jupyterlab-lsp/src/extractors/types.ts index 46e7ccb6f..4b141a7e1 100644 --- a/packages/jupyterlab-lsp/src/extractors/types.ts +++ b/packages/jupyterlab-lsp/src/extractors/types.ts @@ -9,6 +9,11 @@ export interface IExtractedCode { * Range of the foreign code relative to the original source. */ range: CodeEditor.IRange; + /** + * Shift due to any additional code inserted at the beginning of the virtual document + * (usually in order to mock the arguments passed to a magic, or to provide other context clues for the linters) + */ + virtual_shift: CodeEditor.IPosition | null; /** * Code to be retained in the virtual document of the host. */ diff --git a/packages/jupyterlab-lsp/src/virtual/document.spec.ts b/packages/jupyterlab-lsp/src/virtual/document.spec.ts index 55c6a6f7c..c2ebf76f0 100644 --- a/packages/jupyterlab-lsp/src/virtual/document.spec.ts +++ b/packages/jupyterlab-lsp/src/virtual/document.spec.ts @@ -1,9 +1,9 @@ import { expect } from 'chai'; -import { RegExpForeignCodeExtractor } from '../extractors/regexp'; import { is_within_range, VirtualDocument } from './document'; import * as CodeMirror from 'codemirror'; import { ISourcePosition, IVirtualPosition } from '../positioning'; import { CodeEditor } from '@jupyterlab/codeeditor'; +import { foreign_code_extractors } from '../extractors/defaults'; let R_LINE_MAGICS = `%R df = data.frame() print("df created") @@ -46,26 +46,18 @@ describe('is_within_range', () => { }); describe('VirtualDocument', () => { - let r_line_extractor_removing = new RegExpForeignCodeExtractor({ - language: 'R', - pattern: '(^|\n)%R (.*)\n?', - extract_to_foreign: '$2', - keep_in_host: false, - is_standalone: false, - file_extension: 'R' - }); let document = new VirtualDocument( 'python', 'test.ipynb', {}, - { python: [r_line_extractor_removing] }, + foreign_code_extractors, false, 'py', false ); describe('#extract_foreign_code', () => { - it('joins non-standalone fragments together for both foreign and host code', () => { + it('joins non-standalone fragments together', () => { let { cell_code_kept, foreign_document_map @@ -74,15 +66,14 @@ describe('VirtualDocument', () => { column: 0 }); - expect(cell_code_kept).to.equal( - 'print("df created")\nprint("plotted")\n' - ); + // note R cell lines are kept in code (keep_in_host=true) + expect(cell_code_kept).to.equal(R_LINE_MAGICS); expect(foreign_document_map.size).to.equal(2); let { virtual_document: r_document } = foreign_document_map.get( foreign_document_map.keys().next().value ); - expect(r_document.language).to.equal('R'); + expect(r_document.language).to.equal('r'); expect(r_document.value).to.equal('df = data.frame()\n\n\nggplot(df)\n'); }); }); @@ -94,14 +85,24 @@ describe('VirtualDocument', () => { let init_document_with_Python_and_R = () => { let cm_editor_for_cell_1 = {} as CodeMirror.Editor; let cm_editor_for_cell_2 = {} as CodeMirror.Editor; + let cm_editor_for_cell_3 = {} as CodeMirror.Editor; + let cm_editor_for_cell_4 = {} as CodeMirror.Editor; document.append_code_block( - 'test line in Python 1\n%R test line in R 1', + 'test line in Python 1\n%R 1st test line in R line magic 1', cm_editor_for_cell_1 ); document.append_code_block( - 'test line in Python 2\n%R test line in R 2', + 'test line in Python 2\n%R 1st test line in R line magic 2', cm_editor_for_cell_2 ); + document.append_code_block( + '%%R\n1st test line in R cell magic 1', + cm_editor_for_cell_3 + ); + document.append_code_block( + '%%R -i imported_variable\n1st test line in R cell magic 2', + cm_editor_for_cell_4 + ); }; describe('transform_virtual_to_editor', () => { @@ -131,14 +132,30 @@ describe('VirtualDocument', () => { ch: 3 } as ISourcePosition); expect(foreign_document).to.not.equal(document); + expect(foreign_document.value).to.equal( + '1st test line in R line magic 1\n\n\n' + + '1st test line in R line magic 2\n\n\n' + + '1st test line in R cell magic 1\n\n\n' + + 'imported_variable <- data.frame(); 1st test line in R cell magic 2\n' + ); - // The second (R) line in the first block - let editor_position = foreign_document.transform_virtual_to_editor({ + // The second (R) line in the first block ("s" in "1st", "1st" in "1st test line in R line magic") + let virtual_r_1_1 = { line: 0, - ch: 0 - } as IVirtualPosition); + ch: 1 + } as IVirtualPosition; + + // For future reference, the code below would be wrong: + // let source_position = foreign_document.transform_virtual_to_source(virtual_r_1_1); + // expect(source_position.line).to.equal(1); + // expect(source_position.ch).to.equal(4); + // because it checks R source position, rather than checking root source positions. + + let editor_position = foreign_document.transform_virtual_to_editor( + virtual_r_1_1 + ); expect(editor_position.line).to.equal(1); - expect(editor_position.ch).to.equal(3); + expect(editor_position.ch).to.equal(4); // The second (R) line in the second block editor_position = foreign_document.transform_virtual_to_editor({ diff --git a/packages/jupyterlab-lsp/src/virtual/document.ts b/packages/jupyterlab-lsp/src/virtual/document.ts index e69a2a09e..8a6bb82b9 100644 --- a/packages/jupyterlab-lsp/src/virtual/document.ts +++ b/packages/jupyterlab-lsp/src/virtual/document.ts @@ -147,7 +147,7 @@ export class VirtualDocument { * Virtual lines keep all the lines present in the document AND extracted to the foreign document. */ public virtual_lines: Map; // probably should go protected - public source_lines: Map; // probably should go protected + protected source_lines: Map; protected foreign_extractors: IForeignCodeExtractor[]; protected overrides_registry: IOverridesRegistry; @@ -417,6 +417,34 @@ export class VirtualDocument { } as IVirtualPosition; } + private choose_foreign_document(extractor: IForeignCodeExtractor) { + let foreign_document: VirtualDocument; + // if not standalone, try to append to existing document + let foreign_exists = this.foreign_documents.has(extractor.language); + if (!extractor.standalone && foreign_exists) { + foreign_document = this.foreign_documents.get(extractor.language); + this.unused_documents.delete(foreign_document); + } else { + // if standalone, try to re-use existing connection to the server + let unused_standalone = this.unused_standalone_documents.get( + extractor.language + ); + if (extractor.standalone && unused_standalone.length > 0) { + foreign_document = unused_standalone.pop(); + this.unused_documents.delete(foreign_document); + } else { + // if (previous document does not exists) or (extractor produces standalone documents + // and no old standalone document could be reused): create a new document + foreign_document = this.open_foreign( + extractor.language, + extractor.standalone, + extractor.file_extension + ); + } + } + return foreign_document; + } + extract_foreign_code( cell_code: string, cm_editor: CodeMirror.Editor, @@ -440,30 +468,7 @@ export class VirtualDocument { for (let result of results) { if (result.foreign_code !== null) { - let foreign_document: VirtualDocument; - // if not standalone, try to append to existing document - let foreign_exists = this.foreign_documents.has(extractor.language); - if (!extractor.standalone && foreign_exists) { - foreign_document = this.foreign_documents.get(extractor.language); - this.unused_documents.delete(foreign_document); - } else { - // if standalone, try to re-use existing connection to the server - let unused_standalone = this.unused_standalone_documents.get( - extractor.language - ); - if (extractor.standalone && unused_standalone.length > 0) { - foreign_document = unused_standalone.pop(); - this.unused_documents.delete(foreign_document); - } else { - // if (previous document does not exists) or (extractor produces standalone documents - // and no old standalone document could be reused): create a new document - foreign_document = this.open_foreign( - extractor.language, - extractor.standalone, - extractor.file_extension - ); - } - } + let foreign_document = this.choose_foreign_document(extractor); foreign_document_map.set(result.range, { virtual_line: foreign_document.last_virtual_line, @@ -476,7 +481,8 @@ export class VirtualDocument { foreign_document.append_code_block( result.foreign_code, cm_editor, - foreign_shift + foreign_shift, + result.virtual_shift ); } if (result.host_code !== null) { @@ -544,7 +550,8 @@ export class VirtualDocument { append_code_block( cell_code: string, cm_editor: CodeMirror.Editor, - editor_shift: CodeEditor.IPosition = { line: 0, column: 0 } + editor_shift: CodeEditor.IPosition = { line: 0, column: 0 }, + virtual_shift?: CodeEditor.IPosition ) { let source_cell_lines = cell_code.split('\n'); @@ -564,8 +571,11 @@ export class VirtualDocument { } for (let i = 0; i < source_cell_lines.length; i++) { this.source_lines.set(this.last_source_line + i, { - editor_line: i + editor_shift.line, - editor_shift: editor_shift, + editor_line: i, + editor_shift: { + line: editor_shift.line, + column: i === 0 ? editor_shift.column : 0 + }, // TODO: move those to a new abstraction layer (DocumentBlock class) editor: cm_editor, foreign_documents_map: foreign_document_map, @@ -669,11 +679,13 @@ export class VirtualDocument { } transform_source_to_editor(pos: ISourcePosition): IEditorPosition { - let editor_line = this.source_lines.get(pos.line).editor_line; - let editor_shift = this.source_lines.get(pos.line).editor_shift; + let source_line = this.source_lines.get(pos.line); + let editor_line = source_line.editor_line; + let editor_shift = source_line.editor_shift; return { - ch: pos.ch + editor_shift.column, - line: editor_line + // only shift column in the first line + ch: pos.ch + (editor_line === 0 ? editor_shift.column : 0), + line: editor_line + editor_shift.line // TODO or: // line: pos.line + editor_shift.line - this.first_line_of_the_block(editor) } as IEditorPosition;