#17019 closed enhancement (fixed)
Make sure preparser for .sage files respects module docstrings
Reported by:  Johan Rosenkilde  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 8 years ago by
Branch:  → u/jsrn/preparser_for__sage_files_no_longer_respects_module_docstrings 

comment:2 followup: 3 Changed 8 years ago by
Commit:  → 9f82a15e2090509b66987a7e3705348dcfa05e0e 

Status:  new → needs_review 
comment:3 Changed 8 years ago by
Status:  needs_review → 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 8 years ago by
Also, you don't need an additional
from sage.misc.preparser import preparse_file
comment:5 Changed 8 years ago by
Commit:  9f82a15e2090509b66987a7e3705348dcfa05e0e → df1d574405ea5f0f84a96bb85138862bafa9785e 

Branch pushed to git repo; I updated commit sha1. New commits:
df1d574  Added doctest. Rm unneccessary import line

comment:6 Changed 8 years ago by
Status:  needs_work → needs_review 

Ok, I've added a doctest for this.
comment:7 Changed 8 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 8 years ago by
Status:  needs_review → needs_work 

Sorry to bother again, but you should rebase this over #16955.
comment:9 Changed 8 years ago by
...and instead of adding a completely new doctest, why not simply change the old test for sage preparse
?
comment:11 Changed 8 years ago by
Commit:  df1d574405ea5f0f84a96bb85138862bafa9785e → 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 8 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 Changed 8 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 8 years ago by
Commit:  bedc186917b9f9719edf4f3de3cebd71d5aa1926 → 88eee97d19a281d4ff00d0a8eba0992ce989f4bf 

Branch pushed to git repo; I updated commit sha1. New commits:
88eee97  Write out the coding line

comment:15 Changed 8 years ago by
Commit:  88eee97d19a281d4ff00d0a8eba0992ce989f4bf → 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 8 years ago by
Status:  needs_work → 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 8 years ago by
Branch:  u/jsrn/preparser_for__sage_files_no_longer_respects_module_docstrings → u/jdemeyer/ticket/17019 

Created:  Sep 20, 2014, 7:16:12 PM → Sep 20, 2014, 7:16:12 PM 
Modified:  Sep 25, 2014, 12:36:45 PM → Sep 25, 2014, 12:36:45 PM 
comment:18 Changed 8 years ago by
Commit:  04bc257623d0c78d1799372c0e4351292c6fc784 → 86350b34b6c40240becb3a4adfc9025fbaba4386 

Branch pushed to git repo; I updated commit sha1. New commits:
86350b3  Fix doctest formatting

comment:19 Changed 8 years ago by
Reviewers:  → Jeroen Demeyer 

Status:  needs_review → 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 8 years ago by
Branch:  u/jdemeyer/ticket/17019 → u/jsrn/ticket/17019 

comment:21 followups: 22 23 Changed 8 years ago by
Branch:  u/jsrn/ticket/17019 → u/jdemeyer/ticket/17019 

Status:  needs_work → 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 Changed 8 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 Changed 8 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 8 years ago by
Branch:  u/jdemeyer/ticket/17019 → u/jsrn/ticket/17019 

comment:25 Changed 8 years ago by
Commit:  86350b34b6c40240becb3a4adfc9025fbaba4386 → 6d8a3303e88859a78a3809d7dc12bcff4306440f 

Description:  modified (diff) 
Status:  needs_info → needs_work 
Summary:  preparser for .sage files no longer respects module docstrings → Make sure preparser for .sage files respects module docstrings 
Type:  defect → 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 8 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 7 years ago by
Commit:  6d8a3303e88859a78a3809d7dc12bcff4306440f → f21f3862591b3de7968f8d552f641f02ab8f28fa 

comment:28 Changed 7 years ago by
Description:  modified (diff) 

Status:  needs_work → 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 7 years ago by
Reviewers:  Jeroen Demeyer → Jeroen Demeyer, Volker Braun 

Status:  needs_review → positive_review 
comment:31 Changed 7 years ago by
Branch:  u/jsrn/ticket/17019 → f21f3862591b3de7968f8d552f641f02ab8f28fa 

Resolution:  → fixed 
Status:  positive_review → closed 
comment:32 Changed 6 years ago by
Authors:  Johan S. R. Nielsen → Johan Sebastian Rosenkilde Nielsen 

Commit:  f21f3862591b3de7968f8d552f641f02ab8f28fa 
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