#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 )
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)
Change History (27)
Changed 10 years ago by
comment:1 Changed 10 years ago by
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 "...".
comment:2 Changed 10 years ago by
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 10 years ago by
OK, that didn't take all that long. All tests pass.
comment:4 Changed 10 years ago by
- 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 10 years ago by
- Description modified (diff)
Just fixed the https://
.
And the Author(s) field. (This should contain the real name(s).)
comment:6 Changed 10 years ago by
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: ↓ 8 Changed 10 years ago by
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 10 years ago by
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 10 years ago by
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: ↓ 11 Changed 10 years ago by
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: ↓ 12 Changed 10 years ago by
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 10 years ago by
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 248 248 """ 249 249 Run the preparser on the documentation string s. 250 250 This *only* preparses the input lines, i.e., those 251 that begin with "sage: ".or with "..."251 that begin with "sage: ".or with "... " 252 252 """ 253 253 sl = s.lower() 254 254 … … 260 260 last_prompt_comment = '' 261 261 262 262 for L in s.splitlines(): 263 begin = L.lstrip()[: 5]263 begin = L.lstrip()[:6] 264 264 comment = '' 265 if begin == 'sage: ':265 if begin == 'sage: ': 266 266 c, comment = comment_modifier(L) 267 267 last_prompt_comment = comment 268 268 line = '' 269 269 if LONG_TIME in c and not long_time: 270 270 L = '\n' # extra line so output ignored 271 271 if RANDOM in c: 272 L = L.replace('sage: ', 'sage: print "ignore this"; ')272 L = L.replace('sage: ', 'sage: print "ignore this"; ') 273 273 if NOT_TESTED in c: 274 274 L = '\n' # not tested 275 275 if NOT_IMPLEMENTED in c: … … 287 287 line += '\n' + ' '*i + 'ignore ...\n' 288 288 t.append(line) 289 289 290 elif begin.startswith('... '):290 elif begin.startswith('... '): 291 291 comment = last_prompt_comment 292 292 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:])) 294 294 295 295 else: 296 296 comment = last_prompt_comment … … 380 380 j = 0 381 381 while j < len(F): 382 382 L = F[j].rstrip() 383 if L.lstrip()[: 5] == 'sage:':383 if L.lstrip()[:6] == 'sage: ': 384 384 while j < len(F) and L.endswith('\\') and not L.endswith('\\\\'): 385 385 j += 1 386 386 i += 1 387 L = L[:-1] + F[j].lstrip().lstrip('... ').rstrip()387 L = L[:-1] + F[j].lstrip().lstrip('... ').rstrip() 388 388 L += '###_sage"line %s:_sage_ %s_sage"'%(i, L.strip()) 389 389 j += 1 390 390 i += 1 … … 478 478 return '' 479 479 s += test_code(os.path.abspath(file_name)) 480 480 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"','') 483 483 484 484 return s 485 485 … … 607 607 cnt += 1 608 608 if cnt > 1000: 609 609 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' 612 612 r = re.compile(c) 613 613 s = r.sub('\n',s) 614 614 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 10 years ago by
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 10 years ago by
ping
comment:15 Changed 10 years ago by
I've added this ticket to the !leet meta-ticket for doctesting issues, #11337. :)
comment:16 follow-up: ↓ 17 Changed 10 years ago by
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: ↓ 18 Changed 10 years ago by
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: ↓ 19 Changed 10 years ago by
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 10 years ago by
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: ↓ 24 Changed 8 years ago by
- 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 8 years ago by
- Status changed from needs_review to positive_review
comment:22 Changed 8 years ago by
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 8 years ago by
- Resolution set to duplicate
- Status changed from positive_review to closed
comment:24 in reply to: ↑ 20 Changed 8 years ago by
comment:25 Changed 6 years ago by
- Description modified (diff)
testing docstring