Doctest framework fails to parse multiline input pasted from sage interactive prompt

[kini]

See this sage-devel thread for some discussion about the changes proposed by this ticket.

After some confusion trying to get a docstring to pass doctesting, I think input line continuations are "not working", in that lines directly pasted from the sage interpreter prompt do not pass doctests when single inputs span multiple lines. Pasted below is some terminal output (analysis continued afterwards):

keshav@esterhazy /opt/sage/devel $ cat test.rst 
EXAMPLE::

    sage: [1,2,3]
    [1, 2, 3]
    sage: [1,
    ....: 2,3
    ....: ]
    [1, 2, 3]
    sage: x=3;\
    ....: x
    3

keshav@esterhazy /opt/sage/devel $ sage -t -verbose test.rst 
sage -t -verbose "4.6/devel/test.rst"                       
Traceback (most recent call last):
  File "/home/keshav/.sage//tmp/.doctest_test.py", line 60, in <module>
    runner=runner)
  File "/opt/sage/local/bin/sagedoctest.py", line 54, in testmod_returning_runner
    runner=runner)
  File "/opt/sage/local/bin/ncadoctest.py", line 1819, in testmod_returning_runner
    for test in finder.find(m, name, globs=globs, extraglobs=extraglobs):
  File "/opt/sage/local/bin/ncadoctest.py", line 839, in find
    self._find(tests, obj, name, module, source_lines, globs, {})
  File "/opt/sage/local/bin/ncadoctest.py", line 893, in _find
    globs, seen)
  File "/opt/sage/local/bin/ncadoctest.py", line 881, in _find
    test = self._get_test(obj, name, module, globs, source_lines)
  File "/opt/sage/local/bin/ncadoctest.py", line 965, in _get_test
    filename, lineno)
  File "/opt/sage/local/bin/ncadoctest.py", line 594, in get_doctest
    return DocTest(self.get_examples(string, name), globs,
  File "/opt/sage/local/bin/ncadoctest.py", line 608, in get_examples
    return [x for x in self.parse(string, name)
  File "/opt/sage/local/bin/ncadoctest.py", line 570, in parse
    self._parse_example(m, name, lineno)
  File "/opt/sage/local/bin/ncadoctest.py", line 628, in _parse_example
    self._check_prompt_blank(source_lines, indent, name, lineno)
  File "/opt/sage/local/bin/ncadoctest.py", line 715, in _check_prompt_blank
    line[indent:indent+3], line))
ValueError: line 10 of the docstring for __main__.example_0 lacks blank after ...: '    ....: Integer(2),Integer(3)'
 [3.1 s]
 
----------------------------------------------------------------------
The following tests failed:


sage -t -verbose "4.6/devel/test.rst" # Exception from doctest framework
Total time for all tests: 3.1 seconds
keshav@esterhazy /opt/sage/devel $ sed "s/\.\.\.:/\.\./" test.rst > test2.rst
keshav@esterhazy /opt/sage/devel $ sage -t -verbose test2.rst 
sage -t -verbose "4.6/devel/test2.rst"                      
Trying:
    set_random_seed(0L)
Expecting nothing
ok
Trying:
    change_warning_output(sys.stdout)
Expecting nothing
ok
Trying:
    [Integer(1),Integer(2),Integer(3)]###line 3:_sage_    >>> [1,2,3]
Expecting:
    [1, 2, 3]
ok
Trying:
    [Integer(1),###line 5:_sage_    >>> [1,
    Integer(2),Integer(3)
    ]
Expecting:
    [1, 2, 3]
ok
Trying:
    x=Integer(3); x###line 10:_sage_    >>> x=3; x
Expecting:
    3
ok
3 items had no tests:
    __main__
    __main__.change_warning_output
    __main__.warning_function
1 items passed all tests:
   5 tests in __main__.example_0
5 tests in 4 items.
5 passed and 0 failed.
Test passed.
 [3.0 s]
 
----------------------------------------------------------------------
All tests passed!
Total time for all tests: 3.0 seconds
keshav@esterhazy /opt/sage/devel $ 

After I replace all "....: " with "... " using sed, everything seems to work fine.

It seems to me that the problem is that sage is not converting "....: " to "... ", which is what the standard python interactive prompt uses as a prefix for continued input lines. However, the sage prompt uses "....: ", so IMHO the doctesting framework should allow for this, so that lines from sage sessions can really be just dumped into a docstring without any further editing.

Component: doctest coverage

Keywords: continuation, multiline input, interactive prompt

Reviewer: Keshav Kini

Issue created by migration from https://trac.sagemath.org/ticket/10458

[kini]

Attachment: test.rst.gz

testing docstring

[sagetrac-mvngu]

comment:1

The Sage command line interface uses IPython, which is responsible for printing line continuation of the form "...:". It could be more appropriate to fix this issue upstream in the IPython project. We could send a patch upstream or find out how to get IPython to do line continuation of the form "...".

[kini]

patch to make sage recognize "....:" continuation line prefixes in doctests

[kini]

comment:2

Attachment: continuation.patch.gz

Well, Sage already replaces "sage:" with ">>>", so why not replace "....:" with "..."? In any case, "....:" looks better, since it is the same length as "sage:", making the line prefix look more like a separate "column", as it should (note that it is "....:" with four periods, not "...:" as you have written).

Here is a patch which seems to work (at least the attached test.rst passes). What do you think? I'm currently running sage -testall with the patch applied to see if anything broke...

I'm new to sage_trac, so forgive me if setting my username as the "Author" was not the correct thing to do.

[kini]

Author: kini

[kini]

comment:3

OK, that didn't take all that long. All tests pass.

[sagetrac-mvngu]

comment:4

This is really a major change to how the doctesting framework behaves. The issue needs to be raised on sage-devel for some discussion.

[nexttime]

comment:5

Just fixed the https://.

And the Author(s) field. (This should contain the real name(s).)

[nexttime]

Changed author from kini to Keshav Kini

[nexttime]

comment:6

On topic:

I would consistently add the trailing blank in the prompt substitutions.

At first glance it seems we currently lose the original (non-preparsed) source code of continuation lines (but also that this isn't a regression).

[kini]

comment:7

I'm not sure what you mean by "consistently add the trailing blank". The substitutions I patched in do not remove or add any further spaces, so no loss of trailing blanks will occur. If there was a blank after a "...", "....:", or "sage:", there will correspondingly be one after the "...", "...", or ">>>" respectively.

Yeah, I'm not too sure why the source code disappears from all but the first line.

[nexttime]

comment:8

Replying to @kini:

I'm not sure what you mean by "consistently add the trailing blank". The substitutions I patched in do not remove or add any further spaces, so no loss of trailing blanks will occur. If there was a blank after a "...", "....:", or "sage:", there will correspondingly be one after the "...", "...", or ">>>" respectively.

I mean add the space to all prompt patterns to match (and of course their substitutes), e.g. line 485 (it's in the comment, but not the code) and line 613 (also in the comment on line 251).

[nexttime]

comment:9

P.S.: The patch lacks a ticket number in the commit message.

We have the convention to prepend (e.g.) #10458 to the commit messages, and also trac_10458- to the filenames of patches we upload.

[kini]

comment:10

Replying to @nexttime:

I mean add the space to all prompt patterns to match (and of course their substitutes), e.g. line 485 (it's in the comment, but not the code) and line 613 (also in the comment on line 251).

Ah, I see. Yes, I left those alone because changing them would change the behavior of the doctesting framework for docstrings containing lines like "sage:dostuff()" or "...dostuff()". Arguably the doctest framework shouldn't be allowing lines like that anyway, but at the moment it does...

Replying to @nexttime:

We have the convention to prepend (e.g.) #10458 to the commit messages, and also trac_10458- to the filenames of patches we upload.

OK, I'll fix the patch up then.

By the way, when changing my patch submission should I be uploading further patch files to layer on top of the one I've got here, or replacing this patch with new patches which diff to 4.6?

[nexttime]

comment:11

Replying to @kini:

Replying to @nexttime:

I mean add the space to all prompt patterns to match (and of course their substitutes), e.g. line 485 (it's in the comment, but not the code) and line 613 (also in the comment on line 251).

Ah, I see. Yes, I left those alone because changing them would change the behavior of the doctesting framework for docstrings containing lines like "sage:dostuff()" or "...dostuff()". Arguably the doctest framework shouldn't be allowing lines like that anyway, but at the moment it does...

Well, that's just another bug (we should fix here, too).

By the way, when changing my patch submission should I be uploading further patch files to layer on top of the one I've got here, or replacing this patch with new patches which diff to 4.6?

Depends on the changes. Unfortunately, ordinary trac users cannot delete (or rename) attachments, not even their own.

So you could upload a new patch with trac_XXXXX- in the filename, and let someone else delete the old one.

In general, it's ok to replace your patch with an updated one (sometimes better than having -v2 etc.); in case you make further changes rather unrelated to the previous ones, it's usually better to attach a second patch which is based on your previous one. (The attachment comment should then say "Apply on top of ...".)

Also, patches should be based on the most recent developer version of Sage, i.e. the current alpha or rc. Unless there are merge conflicts (because someone else has changed code near to your changes), it's ok to have it based on some previous official release, too. (But you should always check your patch applies clean to the latest developer release.)

The attachment comment should contain the name of the repository to which to apply the patch (the "default" is the Sage library), in this case "Scripts repo", and usually also mentions on what Sage release it is based, since it is not unusual new releases get out before a ticket gets merged.

[nexttime]

comment:12

Replying to @nexttime:

Replying to @kini:

Replying to @nexttime:

I mean add the space to all prompt patterns to match (and of course their substitutes), e.g. line 485 (it's in the comment, but not the code) and line 613 (also in the comment on line 251).

Ah, I see. Yes, I left those alone because changing them would change the behavior of the doctesting framework for docstrings containing lines like "sage:dostuff()" or "...dostuff()". Arguably the doctest framework shouldn't be allowing lines like that anyway, but at the moment it does...

Well, that's just another bug (we should fix here, too).

I've applied the following patch to (the vanilla) sage-doctest and all tests passed (Sage 4.6.1.alpha3, ptestlong), so these changes are not only reasonable or desirable, but also safe: ;-)

diff --git a/sage-doctest b/sage-doctest
--- a/sage-doctest
+++ b/sage-doctest
@@ -248,7 +248,7 @@
     """
     Run the preparser on the documentation string s.
     This *only* preparses the input lines, i.e., those
-    that begin with "sage:".or with "..."
+    that begin with "sage: ".or with "... "
     """
     sl = s.lower()
 
@@ -260,16 +260,16 @@
     last_prompt_comment = ''
     
     for L in s.splitlines():
-        begin = L.lstrip()[:5]
+        begin = L.lstrip()[:6]
         comment = ''
-        if begin == 'sage:':
+        if begin == 'sage: ':
             c, comment = comment_modifier(L)
             last_prompt_comment = comment
             line = ''
             if LONG_TIME in c and not long_time:
                 L = '\n'  # extra line so output ignored
             if RANDOM in c:
-                L = L.replace('sage:', 'sage: print "ignore this"; ') 
+                L = L.replace('sage: ', 'sage: print "ignore this"; ') 
             if NOT_TESTED in c:
                 L = '\n'   # not tested
             if NOT_IMPLEMENTED in c:
@@ -287,10 +287,10 @@
                 line += '\n' + ' '*i + 'ignore ...\n'
             t.append(line)
             
-        elif begin.startswith('...'):
+        elif begin.startswith('... '):
             comment = last_prompt_comment
             i = L.find('.')
-            t.append(L[:i+3] + sage.misc.preparser.preparse(L[i+3:]))
+            t.append(L[:i+4] + sage.misc.preparser.preparse(L[i+4:]))
             
         else:
             comment = last_prompt_comment
@@ -380,11 +380,11 @@
     j = 0
     while j < len(F):
         L = F[j].rstrip()
-        if L.lstrip()[:5] == 'sage:':
+        if L.lstrip()[:6] == 'sage: ':
             while j < len(F) and L.endswith('\\') and not L.endswith('\\\\'):
                 j += 1
                 i += 1
-                L = L[:-1] + F[j].lstrip().lstrip('...').rstrip()
+                L = L[:-1] + F[j].lstrip().lstrip('... ').rstrip()
             L += '###_sage"line %s:_sage_    %s_sage"'%(i, L.strip())
         j += 1
         i += 1
@@ -478,8 +478,8 @@
         return  ''
     s += test_code(os.path.abspath(file_name))
 
-    # Allow for "sage:" instead of the traditional Python ">>>".
-    s = s.replace("sage:",">>>").replace('_sage"','')
+    # Allow for "sage: " instead of the traditional Python ">>> ".
+    s = s.replace("sage: ",">>> ").replace('_sage"','')
     
     return s
 
@@ -607,8 +607,8 @@
         cnt += 1
         if cnt > 1000:
             break
-    s = s.replace(':_sage_',':\n').replace('>>>','sage:')
-    c = '###line [0-9]*\n'
+    s = s.replace(':_sage_',':\n').replace('>>> ','sage: ')
+    c = '###line [0-9]+\n'
     r = re.compile(c)
     s = r.sub('\n',s)
     if cnt > 0:

(Note that there are more occurrences than I had found in your patch / mentioned above.)

So I would suggest to [re]base your enhancement (accepting "....: " line continuations) on these changes, in which case I would attach a proper Mercurial patch for my changes.

After that we could address the "lost original source code of continuation lines" issue, with another patch, perhaps on a follow-up ticket if you like.

[nexttime]

comment:13

P.S.:

We already had trouble with lines starting with "..." interpreted as continuation lines, though they were meant as "don't care" output of a test (since ellipsis is meant to match anything / partial random output - like e.g. line numbers in tracebacks - of a doctest).

[nexttime]

comment:14

ping

[nexttime]

comment:15

I've added this ticket to the !leet meta-ticket for doctesting issues, #11337. :)

[nexttime]

comment:16

Another, slightly related issue is that you can't simply copy tracebacks from the sage: prompt into docstrings because they look a bit different in the doctest framework (and specific line numbers should of course be avoided in a doctest). This ticket certainly won't fix that though.

But we could document (in the Developer's Guide) that one has to strip the exception type, i.e. anything up to Traceback, and replace anything containing filenames or line numbers by ..., still on another ticket I think.

[rbeezer]

comment:17

Replying to @nexttime:

Another, slightly related issue is that you can't simply copy tracebacks from the sage: prompt into docstrings because they look a bit different in the doctest framework (and specific line numbers should of course be avoided in a doctest). This ticket certainly won't fix that though.

And someplace the first line of a traceback lacks a colon, and a doctest needs one. And maybe in the notebook it is different yet again? Or something like that.

See semi-related, new ticket #11621. Does it belong in the meta-ticket? I tripped over it as part of doctesting Sage examples placed into a textbook where I need to be careful about long lines for the printed version.

[nexttime]

comment:18

Replying to @rbeezer:

See semi-related, new ticket #11621. Does it belong in the meta-ticket?

As it only deals with the preparser and not the doctesting framework, I don't think so.

I tripped over it as part of doctesting Sage examples placed into a textbook where I need to be careful about long lines for the printed version.

Release date / deadline? ;-)

[rbeezer]

comment:19

Replying to @nexttime:

Release date / deadline? ;-)

Open source. So, no deadline.

Release date: maybe within the week. Watch sage-edu/sage-devel.

I solved my immediate problem by breaking the input into pieces. Not as pretty as it could be, but not bad either.

Rob

[jdemeyer]

comment:20

I think this is fixed by #12415.

[jdemeyer]

Reviewer: Keshav Kini

[jdemeyer]

Changed author from Keshav Kini to none

[kini]

comment:22

Awesome! :) A bit sad that the first patch I ever wrote for Sage is going to be thrown away, but it was a mere bandaid compared to #12415's cyborgization :P

[nexttime]

comment:24

Replying to @jdemeyer:

I think this is fixed by #12415.

See also #14512.