Merge pull request #206 from krassowski/tests/rpy2

Address some issues with positioning system in foreign documents

Author: krassowski

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
 

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<CodeEditor.IRange, IVirtualDocumentBlock>
   ) {
     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<CodeEditor.IRange, IVirtualDocumentBlock>
+  ) {
+    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'
       );

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'
     }),

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

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.
    */

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({