Merge pull request #346 from krassowski/fix-ipython-overrides

Fix IPython overrides

Author: krassowski

CHANGELOG.md

@@ -1,5 +1,14 @@
 ## CHANGELOG
 
+### `@krassowski/jupyterlab-lsp 2.0.3` (unreleased)
+
+- bug fixes
+
+  - improve code overrides for IPython line magics ([#346])
+  - implement missing code overrides for IPython's pinfo (`?`) and pinfo2 (`??`) syntactic sugar ([#346])
+
+[#346]: https://github.com/krassowski/jupyterlab-lsp/issues/346
+
 ### `@krassowski/jupyterlab-lsp 2.0.2` (2020-09-07)
 
 - bug fixes

packages/jupyterlab-lsp/src/transclusions/ipython-rpy2/overrides.spec.ts

@@ -76,6 +76,12 @@ describe('rpy2 IPython overrides', () => {
       expect(reverse).to.equal(line);
     });
 
+    it('does not substitute magic-like constructs', () => {
+      let line = 'print("%R -i x")';
+      let override = line_magics.override_for(line);
+      expect(override).to.equal(null);
+    });
+
     it('works with the long form arguments', () => {
       let line = '%R --input x';
       let override = line_magics.override_for(line);

packages/jupyterlab-lsp/src/transclusions/ipython-rpy2/overrides.ts

@@ -6,14 +6,18 @@ import {
   rpy2_reverse_pattern,
   rpy2_reverse_replacement
 } from './rpy2';
+import { LINE_MAGIC_PREFIX } from '../ipython/overrides';
 
 export let overrides: IScopedCodeOverride[] = [
   {
     // support up to 10 arguments
-    pattern: '%R' + rpy2_args_pattern(RPY2_MAX_ARGS) + '(.*)(\n)?',
-    replacement: (match, ...args) => {
+    pattern:
+      LINE_MAGIC_PREFIX + '%R' + rpy2_args_pattern(RPY2_MAX_ARGS) + '(.*)(\n)?',
+    replacement: (match, prefix, ...args) => {
       let r = parse_r_args(args, -4);
-      return `${r.outputs}rpy2.ipython.rmagic.RMagics.R("${r.content}", "${r.others}"${r.inputs})`;
+      // note: only supports assignment or -o/--output, not both
+      // TODO assignment like in x = %R 1 should be distinguished from -o
+      return `${prefix}${r.outputs}rpy2.ipython.rmagic.RMagics.R("${r.content}", "${r.others}"${r.inputs})`;
     },
     scope: 'line',
     reverse: {

packages/jupyterlab-lsp/src/transclusions/ipython/overrides.spec.ts

@@ -104,16 +104,30 @@ describe('Default IPython overrides', () => {
       expect(reverse).to.equal(LINE_MAGIC_WITH_SPACE);
     });
 
-    it('overrides x =%ls', () => {
+    it('overrides x =%ls and x = %ls', () => {
       // this is a corner-case as described in
       // https://github.com/krassowski/jupyterlab-lsp/issues/281#issuecomment-645286076
       let override = line_magics_map.override_for('x =%ls');
       expect(override).to.equal('x =get_ipython().run_line_magic("ls", "")');
+
+      override = line_magics_map.override_for('x = %ls');
+      expect(override).to.equal('x = get_ipython().run_line_magic("ls", "")');
     });
 
     it('does not override line-magic-like constructs', () => {
       let override = line_magics_map.override_for('list("%")');
       expect(override).to.equal(null);
+
+      override = line_magics_map.override_for('list(" %test")');
+      expect(override).to.equal(null);
+    });
+
+    it('does not override modulo operators', () => {
+      let override = line_magics_map.override_for('3 % 2');
+      expect(override).to.equal(null);
+
+      override = line_magics_map.override_for('3%2');
+      expect(override).to.equal(null);
     });
 
     it('escapes arguments', () => {
@@ -135,9 +149,62 @@ describe('Default IPython overrides', () => {
       expect(reverse).to.equal('!ls -o');
     });
 
+    it('overrides shell commands assignments: x =!ls and x = !ls', () => {
+      let override = line_magics_map.override_for('x =!ls');
+      expect(override).to.equal('x =get_ipython().getoutput("ls")');
+
+      override = line_magics_map.override_for('x = !ls');
+      expect(override).to.equal('x = get_ipython().getoutput("ls")');
+    });
+
     it('does not override shell-like constructs', () => {
       let override = line_magics_map.override_for('"!ls"');
       expect(override).to.equal(null);
     });
+
+    it('does not override != comparisons', () => {
+      let override = line_magics_map.override_for('1 != None');
+      expect(override).to.equal(null);
+    });
+  });
+
+  describe('IPython help line magics', () => {
+    let line_magics_map = new ReversibleOverridesMap(
+      overrides.filter(override => override.scope == 'line')
+    );
+
+    it('overrides pinfo', () => {
+      let override = line_magics_map.override_for('?int');
+      expect(override).to.equal("get_ipython().run_line_magic('pinfo', 'int')");
+
+      let reverse = line_magics_map.reverse.override_for(override);
+      expect(reverse).to.equal('?int');
+
+      override = line_magics_map.override_for('int?');
+      expect(override).to.equal(
+        "get_ipython().run_line_magic('pinfo',  'int')"
+      );
+
+      reverse = line_magics_map.reverse.override_for(override);
+      expect(reverse).to.equal('int?');
+    });
+
+    it('overrides pinfo2', () => {
+      let override = line_magics_map.override_for('??int');
+      expect(override).to.equal(
+        "get_ipython().run_line_magic('pinfo2', 'int')"
+      );
+
+      let reverse = line_magics_map.reverse.override_for(override);
+      expect(reverse).to.equal('??int');
+
+      override = line_magics_map.override_for('int??');
+      expect(override).to.equal(
+        "get_ipython().run_line_magic('pinfo2',  'int')"
+      );
+
+      reverse = line_magics_map.reverse.override_for(override);
+      expect(reverse).to.equal('int??');
+    });
   });
 });

packages/jupyterlab-lsp/src/transclusions/ipython/overrides.ts

@@ -16,20 +16,40 @@ function empty_or_escaped(x: string) {
   }
 }
 
+/**
+ * Line magics do not have to start with the new line, for example:
+ *    x = !ls
+ *    x = %ls
+ *    x =%ls
+ * are all valid.
+ *
+ * The percent may also appear in strings, e.g. ls('%').
+ *
+ * IPython allows magics on start of a line or in assignments (but only there!), thus:
+ *    x = (!ls)
+ * is invalid syntax!
+ *
+ * Therefore we can require that the match starts with either:
+ * - zero or more whitespaces right after the beginning of a line, or
+ * - variable then equals (with optional whitespaces)
+ *
+ * This will not always work: e.g.:
+ *    x['a = !ls'] = !ls
+ * is perfectly valid IPython, but regular expressions cannot help here.
+ *
+ * Look behind could be used to avoid capturing the group,
+ * but at the time of writing support is only at 77%.
+ */
+export const LINE_MAGIC_PREFIX = '^(\\s*|\\s*\\S+\\s*=\\s*)';
+
 export let overrides: IScopedCodeOverride[] = [
   /**
    * Line magics
    */
   // filter out IPython line magics and shell assignments:
   //  keep the content, keep magic/command name and new line at the end
-
-  // note magics do not have to start with the new line, for example x = !ls or x = %ls are both valid.
-  // x =%ls is also valid. However, percent may also appear in strings, e.g. ls('%').
-  // Hence: (^|\\s|=) for shell commands (!) and line magics (%); see issue #281.
-  // This does not solve all issues, for example `list(" %ls")` still leads to:
-  // `list(" get_ipython().run_line_magic("ls")", "")`.
   {
-    pattern: '(^|\\s|=)!(\\S+)(.*)(\n)?',
+    pattern: LINE_MAGIC_PREFIX + '!([^=\\s]+)(.*)(\n)?',
     replacement: '$1get_ipython().getoutput("$2$3")$4',
     scope: 'line',
     reverse: {
@@ -39,7 +59,46 @@ export let overrides: IScopedCodeOverride[] = [
     }
   },
   {
-    pattern: '(^|\\s|=)%(\\S+)(.*)(\n)?',
+    // note: assignments of pinfo/pinfo2 are not supported by IPython
+    pattern: '^(\\s*)' + '(\\?{1,2})(\\S+)(\n)?',
+    replacement: (match, prefix, marks, name, line_break) => {
+      const cmd = marks == '?' ? 'pinfo' : 'pinfo2';
+      line_break = line_break || '';
+      // trick: use single quotes to distinguish
+      return `${prefix}get_ipython().run_line_magic(\'${cmd}\', \'${name}\')${line_break}`;
+    },
+    scope: 'line',
+    reverse: {
+      pattern:
+        "get_ipython\\(\\).run_line_magic\\('(pinfo2?)', '(.*?)'\\)(\n)?",
+      replacement: (match, cmd, name) => {
+        const marks = cmd == 'pinfo' ? '?' : '??';
+        return `${marks}${name}`;
+      },
+      scope: 'line'
+    }
+  },
+  {
+    pattern: '^(\\s*)' + '([^\\?\\s]+)(\\?{1,2})(\n)?',
+    replacement: (match, prefix, name, marks, line_break) => {
+      const cmd = marks == '?' ? 'pinfo' : 'pinfo2';
+      line_break = line_break || '';
+      // trick: use two spaces to distinguish pinfo using suffix (int?) from the one using prefix (?int)
+      return `${prefix}get_ipython().run_line_magic(\'${cmd}\',  \'${name}\')${line_break}`;
+    },
+    scope: 'line',
+    reverse: {
+      pattern:
+        "get_ipython\\(\\).run_line_magic\\('(pinfo2?)',  '(.*?)'\\)(\n)?",
+      replacement: (match, cmd, name) => {
+        const marks = cmd == 'pinfo' ? '?' : '??';
+        return `${name}${marks}`;
+      },
+      scope: 'line'
+    }
+  },
+  {
+    pattern: LINE_MAGIC_PREFIX + '%(\\S+)(.*)(\n)?',
     replacement: (match, prefix, name, args, line_break) => {
       args = empty_or_escaped(args);
       line_break = line_break || '';