#17019 closed enhancement (fixed)
Make sure preparser for .sage files respects module docstrings
Reported by:  jsrn  Owned by:  

Priority:  minor  Milestone:  sage6.4 
Component:  misc  Keywords:  docstring, preparse 
Cc:  Merged in:  
Authors:  Johan Sebastian Rosenkilde Nielsen  Reviewers:  Jeroen Demeyer, Volker Braun 
Report Upstream:  N/A  Work issues:  
Branch:  f21f386 (Commits, GitHub, GitLab)  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
Running ./sage preparse
on a .sage
produces a .py
file such that this is a proper Python file. If the .sage file contains a module docstring, then this should be present as the module docstring of the resulting .py file. This means that no codelines (noncomment, nonblank) may be added before this module docstring.
There are no doctests for this (and incidentally, also no doctests that sage preparse
respects the encoding line), and the way sagepreparse
is currently written is quite fragile with respect to changes in called code (in sage.misc.preparser
).
Change History (32)
comment:1 Changed 7 years ago by
 Branch set to u/jsrn/preparser_for__sage_files_no_longer_respects_module_docstrings
comment:2 followup: ↓ 3 Changed 7 years ago by
 Commit set to 9f82a15e2090509b66987a7e3705348dcfa05e0e
 Status changed from new to needs_review
comment:3 in reply to: ↑ 2 Changed 7 years ago by
 Status changed from needs_review to needs_work
Replying to jsrn:
Since the patch is on a python file outside the usual Sage tree, I am not aware of doctest rules etc. I wouldn't even know how to make a doctest for this (and there are no other doctests in sagepreparse).
There are doctest (even for sagepreparse
) in src/sage/tests/cmdline.py
and I think you should add a doctest for this (needs_work because of this).
comment:4 Changed 7 years ago by
Also, you don't need an additional
from sage.misc.preparser import preparse_file
comment:5 Changed 7 years ago by
 Commit changed from 9f82a15e2090509b66987a7e3705348dcfa05e0e to df1d574405ea5f0f84a96bb85138862bafa9785e
Branch pushed to git repo; I updated commit sha1. New commits:
df1d574  Added doctest. Rm unneccessary import line

comment:6 Changed 7 years ago by
 Status changed from needs_work to needs_review
Ok, I've added a doctest for this.
comment:7 Changed 7 years ago by
The Buildbot's doctests failing is in some numerical stability tests, and seem to have nothing to do with this patch.
comment:8 Changed 7 years ago by
 Status changed from needs_review to needs_work
Sorry to bother again, but you should rebase this over #16955.
comment:9 Changed 7 years ago by
...and instead of adding a completely new doctest, why not simply change the old test for sage preparse
?
comment:10 Changed 7 years ago by
You removed support for coding
.
comment:11 Changed 7 years ago by
 Commit changed from df1d574405ea5f0f84a96bb85138862bafa9785e to bedc186917b9f9719edf4f3de3cebd71d5aa1926
Branch pushed to git repo; I updated commit sha1. New commits:
4004f1f  Merge branch 'u/jdemeyer/ticket/16827' of git://trac.sagemath.org/sage into ticket/16955

17ad642  Preparse file foo.sage to foo.sage.py

255b55e  Merge commit '17ad642b0f01e6411b2edf444ce53ea1813bacf3' into HEAD

bedc186  Fixed according to 16995

comment:12 followup: ↓ 13 Changed 7 years ago by
I tried to do the rebase, but the instructions in the documentation were not clear to me on this usecase. Did I do it right?
I don't know what you mean with "removed support for coding
".
I added a new doctest instead of making the old one bigger because I didn't want the old one to be too large and confusing (as it is, it is hard to read). In the new doctest is an import inspect
and a strange line printing out the doc string.
comment:13 in reply to: ↑ 12 Changed 7 years ago by
Replying to jsrn:
I tried to do the rebase, but the instructions in the documentation were not clear to me on this usecase. Did I do it right?
I guess so, at least I don't see anything obviously bad.
I don't know what you mean with "removed support for
coding
".
grep src/bin/sagepreparse
for coding
and you will see what I mean.
comment:14 Changed 7 years ago by
 Commit changed from bedc186917b9f9719edf4f3de3cebd71d5aa1926 to 88eee97d19a281d4ff00d0a8eba0992ce989f4bf
Branch pushed to git repo; I updated commit sha1. New commits:
88eee97  Write out the coding line

comment:15 Changed 7 years ago by
 Commit changed from 88eee97d19a281d4ff00d0a8eba0992ce989f4bf to 04bc257623d0c78d1799372c0e4351292c6fc784
Branch pushed to git repo; I updated commit sha1. New commits:
04bc257  Also test coding is preserved, and coalesce into one simpler test for preparse

comment:16 Changed 7 years ago by
 Status changed from needs_work to needs_review
Good catch with the coding line. I reinstated it, and added it to the doctest.
I also simplified the doctest and simply examine the produced file directly instead of doing strange reflection stuff.
comment:17 Changed 7 years ago by
 Branch changed from u/jsrn/preparser_for__sage_files_no_longer_respects_module_docstrings to u/jdemeyer/ticket/17019
 Created changed from 09/20/14 19:16:12 to 09/20/14 19:16:12
 Modified changed from 09/25/14 12:36:45 to 09/25/14 12:36:45
comment:18 Changed 7 years ago by
 Commit changed from 04bc257623d0c78d1799372c0e4351292c6fc784 to 86350b34b6c40240becb3a4adfc9025fbaba4386
Branch pushed to git repo; I updated commit sha1. New commits:
86350b3  Fix doctest formatting

comment:19 Changed 7 years ago by
 Reviewers set to Jeroen Demeyer
 Status changed from needs_review to needs_work
I think the doctest is not good enough, since it passes even without the changes to sagepreparse
. If you want to convince me that this ticket is needed, show me a doctest which failed before and passes now.
comment:20 Changed 7 years ago by
 Branch changed from u/jdemeyer/ticket/17019 to u/jsrn/ticket/17019
comment:21 followups: ↓ 22 ↓ 23 Changed 7 years ago by
 Branch changed from u/jsrn/ticket/17019 to u/jdemeyer/ticket/17019
 Status changed from needs_work to needs_info
No need to get snappy  obviously the problem was there or I wouldn't waste my time on this. It seems, however, that somewhere between 6.2 and 6.3 the generation of the _sage_const_X lines were moved, and this presents an alternative solution to the problem (those lines were previously put before the module docstring).
Now for this ticket: the change in sagepreparse could be rolled back since it is not necessary for the moment. The doctest changes could be retained since it actually tests something which was not tested before. The latest test that you saw was however rather silly, since I forgot to reinstate a constant in the code after I merged the two doctests into one. I've fixed the doc test...
comment:22 in reply to: ↑ 21 Changed 7 years ago by
Replying to jsrn:
Now for this ticket: the change in sagepreparse could be rolled back since it is not necessary for the moment.
I didn't say the changes were bad, in fact I do think they make sense. I only said that I could not check whether they solved the problem mentioned in this ticket.
comment:23 in reply to: ↑ 21 Changed 7 years ago by
Replying to jsrn:
I've fixed the doc test...
Then please push your branch...
Also: can you update the ticket description please, since the problem is no longer applicable.
comment:24 Changed 7 years ago by
 Branch changed from u/jdemeyer/ticket/17019 to u/jsrn/ticket/17019
comment:25 Changed 7 years ago by
 Commit changed from 86350b34b6c40240becb3a4adfc9025fbaba4386 to 6d8a3303e88859a78a3809d7dc12bcff4306440f
 Description modified (diff)
 Status changed from needs_info to needs_work
 Summary changed from preparser for .sage files no longer respects module docstrings to Make sure preparser for .sage files respects module docstrings
 Type changed from defect to enhancement
I'm sorry but I'm completely lost in these branches. I'm working on branch t/17019/ticket/17019 but if I try to push with git push trac t/17019/ticket/17019, I get the following error
Total 0 (delta 0), reused 0 (delta 0) remote: FATAL: W refs/heads/t/17019/ticket/17019 sage jsrn DENIED by fallthru remote: error: hook declined to update refs/heads/t/17019/ticket/17019 To git@trac.sagemath.org:sage.git ! [remote rejected] t/17019/ticket/17019 > t/17019/ticket/17019 (hook declined) error: failed to push some refs to 'git@trac.sagemath.org:sage.git'
If I try to push with git trac push, it works fine but pushes to u/jsrn/ticket/17019, which doesn't seem to put my code into this ticket (but it does add the message "Branch changed from blablabla" which appears twice above).
What do I do?
New commits:
6d8a330  Put a constant in the test

comment:26 Changed 7 years ago by
Ok, now I'm really confused: with 6 minutes delay the commit came through??? Did I do it right or was I just lucky and abusive towards the system?
comment:27 Changed 6 years ago by
 Commit changed from 6d8a3303e88859a78a3809d7dc12bcff4306440f to f21f3862591b3de7968f8d552f641f02ab8f28fa
comment:28 Changed 6 years ago by
 Description modified (diff)
 Status changed from needs_work to needs_review
I'm reviving this: Sage has displayed the docmangling behaviour after preprocessing in more or less all releases since this ticket was last discussed, possibly except the one Jeroen tested on a year ago. I don't know why that was.
At least, I've just tested that the doctest in this ticket fails if it's tested on the current beta, and that this patch fixes it.
comment:29 Changed 5 years ago by
 Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, Volker Braun
 Status changed from needs_review to positive_review
comment:30 Changed 5 years ago by
Necroreview  thanks! :)
comment:31 Changed 5 years ago by
 Branch changed from u/jsrn/ticket/17019 to f21f3862591b3de7968f8d552f641f02ab8f28fa
 Resolution set to fixed
 Status changed from positive_review to closed
comment:32 Changed 5 years ago by
 Commit f21f3862591b3de7968f8d552f641f02ab8f28fa deleted
Since the patch is on a python file outside the usual Sage tree, I am not aware of doctest rules etc. I wouldn't even know how to make a doctest for this (and there are no other doctests in sagepreparse).
New commits:
Simple change to sagepreparse to make sure the module docstring is put first
Merge w. atomic_write change from 6.3