Opened 9 years ago

Closed 7 years ago

Last modified 4 years ago

#10458 closed defect (duplicate)

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

Reported by: kini Owned by: mvngu
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: doctest coverage Keywords: doctest, continuation, multiline input, interactive prompt
Cc: Merged in:
Authors: Reviewers: Keshav Kini
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by chapoton)

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.

Attachments (2)

test.rst (132 bytes) - added by kini 9 years ago.
testing docstring
continuation.patch (2.2 KB) - added by kini 9 years ago.
patch to make sage recognize "....:" continuation line prefixes in doctests

Download all attachments as: .zip

Change History (27)

Changed 9 years ago by kini

testing docstring

comment:1 Changed 9 years ago by mvngu

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 "...".

Changed 9 years ago by kini

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

comment:2 Changed 9 years ago by kini

  • Authors set to kini

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.

comment:3 Changed 9 years ago by kini

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

comment:4 Changed 9 years ago by mvngu

  • Description modified (diff)

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

comment:5 Changed 9 years ago by leif

  • Authors changed from kini to Keshav Kini
  • Description modified (diff)

Just fixed the https://.

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

comment:6 Changed 9 years ago by leif

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).

comment:7 follow-up: Changed 9 years ago by 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.

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

comment:8 in reply to: ↑ 7 Changed 9 years ago by leif

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).

comment:9 Changed 9 years ago by leif

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.

comment:10 follow-up: Changed 9 years ago by kini

Replying to leif:

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 leif:

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?

comment:11 in reply to: ↑ 10 ; follow-up: Changed 9 years ago by leif

Replying to kini:

Replying to leif:

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.

comment:12 in reply to: ↑ 11 Changed 9 years ago by leif

Replying to leif:

Replying to kini:

Replying to leif:

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: ;-)

  • sage-doctest

    diff --git a/sage-doctest b/sage-doctest
    a b  
    248248    """
    249249    Run the preparser on the documentation string s.
    250250    This *only* preparses the input lines, i.e., those
    251     that begin with "sage:".or with "..."
     251    that begin with "sage: ".or with "... "
    252252    """
    253253    sl = s.lower()
    254254
     
    260260    last_prompt_comment = ''
    261261   
    262262    for L in s.splitlines():
    263         begin = L.lstrip()[:5]
     263        begin = L.lstrip()[:6]
    264264        comment = ''
    265         if begin == 'sage:':
     265        if begin == 'sage: ':
    266266            c, comment = comment_modifier(L)
    267267            last_prompt_comment = comment
    268268            line = ''
    269269            if LONG_TIME in c and not long_time:
    270270                L = '\n'  # extra line so output ignored
    271271            if RANDOM in c:
    272                 L = L.replace('sage:', 'sage: print "ignore this"; ')
     272                L = L.replace('sage: ', 'sage: print "ignore this"; ')
    273273            if NOT_TESTED in c:
    274274                L = '\n'   # not tested
    275275            if NOT_IMPLEMENTED in c:
     
    287287                line += '\n' + ' '*i + 'ignore ...\n'
    288288            t.append(line)
    289289           
    290         elif begin.startswith('...'):
     290        elif begin.startswith('... '):
    291291            comment = last_prompt_comment
    292292            i = L.find('.')
    293             t.append(L[:i+3] + sage.misc.preparser.preparse(L[i+3:]))
     293            t.append(L[:i+4] + sage.misc.preparser.preparse(L[i+4:]))
    294294           
    295295        else:
    296296            comment = last_prompt_comment
     
    380380    j = 0
    381381    while j < len(F):
    382382        L = F[j].rstrip()
    383         if L.lstrip()[:5] == 'sage:':
     383        if L.lstrip()[:6] == 'sage: ':
    384384            while j < len(F) and L.endswith('\\') and not L.endswith('\\\\'):
    385385                j += 1
    386386                i += 1
    387                 L = L[:-1] + F[j].lstrip().lstrip('...').rstrip()
     387                L = L[:-1] + F[j].lstrip().lstrip('... ').rstrip()
    388388            L += '###_sage"line %s:_sage_    %s_sage"'%(i, L.strip())
    389389        j += 1
    390390        i += 1
     
    478478        return  ''
    479479    s += test_code(os.path.abspath(file_name))
    480480
    481     # Allow for "sage:" instead of the traditional Python ">>>".
    482     s = s.replace("sage:",">>>").replace('_sage"','')
     481    # Allow for "sage: " instead of the traditional Python ">>> ".
     482    s = s.replace("sage: ",">>> ").replace('_sage"','')
    483483   
    484484    return s
    485485
     
    607607        cnt += 1
    608608        if cnt > 1000:
    609609            break
    610     s = s.replace(':_sage_',':\n').replace('>>>','sage:')
    611     c = '###line [0-9]*\n'
     610    s = s.replace(':_sage_',':\n').replace('>>> ','sage: ')
     611    c = '###line [0-9]+\n'
    612612    r = re.compile(c)
    613613    s = r.sub('\n',s)
    614614    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.

comment:13 Changed 9 years ago by leif

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).

comment:14 Changed 8 years ago by leif

ping

comment:15 Changed 8 years ago by leif

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

comment:16 follow-up: Changed 8 years ago by leif

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.

comment:17 in reply to: ↑ 16 ; follow-up: Changed 8 years ago by rbeezer

Replying to leif:

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.

comment:18 in reply to: ↑ 17 ; follow-up: Changed 8 years ago by leif

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? ;-)

comment:19 in reply to: ↑ 18 Changed 8 years ago by rbeezer

Replying to leif:

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

comment:20 follow-up: Changed 7 years ago by jdemeyer

  • Authors Keshav Kini deleted
  • Milestone set to sage-duplicate/invalid/wontfix
  • Reviewers set to Keshav Kini
  • Status changed from new to needs_review

I think this is fixed by #12415.

comment:21 Changed 7 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:22 Changed 7 years ago by kini

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

comment:23 Changed 7 years ago by jdemeyer

  • Resolution set to duplicate
  • Status changed from positive_review to closed

comment:24 in reply to: ↑ 20 Changed 7 years ago by leif

Replying to jdemeyer:

I think this is fixed by #12415.

See also #14512.

comment:25 Changed 4 years ago by chapoton

  • Description modified (diff)
Note: See TracTickets for help on using tickets.