Opened 7 years ago

Closed 5 years ago

Last modified 5 years ago

#17019 closed enhancement (fixed)

Make sure preparser for .sage files respects module docstrings

Reported by: jsrn Owned by:
Priority: minor Milestone: sage-6.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:

Status badges

Description (last modified by jsrn)

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 code-lines (non-comment, non-blank) may be added before this module docstring.

There are no doc-tests for this (and incidentally, also no doc-tests that sage --preparse respects the encoding line), and the way sage-preparse 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 jsrn

  • Branch set to u/jsrn/preparser_for__sage_files_no_longer_respects_module_docstrings

comment:2 follow-up: Changed 7 years ago by jsrn

  • Commit set to 9f82a15e2090509b66987a7e3705348dcfa05e0e
  • Status changed from new to needs_review

Since the patch is on a python file outside the usual Sage tree, I am not aware of doc-test rules etc. I wouldn't even know how to make a doctest for this (and there are no other doctests in sage-preparse).


New commits:

f9f9727Simple change to sage-preparse to make sure the module docstring is put first
9f82a15Merge w. atomic_write change from 6.3

comment:3 in reply to: ↑ 2 Changed 7 years ago by jdemeyer

  • 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 doc-test rules etc. I wouldn't even know how to make a doctest for this (and there are no other doctests in sage-preparse).

There are doctest (even for sage-preparse) 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 jdemeyer

Also, you don't need an additional

from sage.misc.preparser import preparse_file

comment:5 Changed 7 years ago by git

  • Commit changed from 9f82a15e2090509b66987a7e3705348dcfa05e0e to df1d574405ea5f0f84a96bb85138862bafa9785e

Branch pushed to git repo; I updated commit sha1. New commits:

df1d574Added doctest. Rm unneccessary import line

comment:6 Changed 7 years ago by jsrn

  • Status changed from needs_work to needs_review

Ok, I've added a doctest for this.

comment:7 Changed 7 years ago by jsrn

The Build-bot'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 jdemeyer

  • 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 jdemeyer

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

You removed support for coding.

comment:11 Changed 7 years ago by git

  • Commit changed from df1d574405ea5f0f84a96bb85138862bafa9785e to bedc186917b9f9719edf4f3de3cebd71d5aa1926

Branch pushed to git repo; I updated commit sha1. New commits:

4004f1fMerge branch 'u/jdemeyer/ticket/16827' of git://trac.sagemath.org/sage into ticket/16955
17ad642Preparse file foo.sage to foo.sage.py
255b55eMerge commit '17ad642b0f01e6411b2edf444ce53ea1813bacf3' into HEAD
bedc186Fixed according to 16995

comment:12 follow-up: Changed 7 years ago by jsrn

I tried to do the rebase, but the instructions in the documentation were not clear to me on this use-case. 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 doc-test is an import inspect and a strange line printing out the doc string.

comment:13 in reply to: ↑ 12 Changed 7 years ago by jdemeyer

Replying to jsrn:

I tried to do the rebase, but the instructions in the documentation were not clear to me on this use-case. 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/sage-preparse for coding and you will see what I mean.

comment:14 Changed 7 years ago by git

  • Commit changed from bedc186917b9f9719edf4f3de3cebd71d5aa1926 to 88eee97d19a281d4ff00d0a8eba0992ce989f4bf

Branch pushed to git repo; I updated commit sha1. New commits:

88eee97Write out the coding line

comment:15 Changed 7 years ago by git

  • Commit changed from 88eee97d19a281d4ff00d0a8eba0992ce989f4bf to 04bc257623d0c78d1799372c0e4351292c6fc784

Branch pushed to git repo; I updated commit sha1. New commits:

04bc257Also test coding is preserved, and coalesce into one simpler test for --preparse

comment:16 Changed 7 years ago by jsrn

  • 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 jdemeyer

  • 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 git

  • Commit changed from 04bc257623d0c78d1799372c0e4351292c6fc784 to 86350b34b6c40240becb3a4adfc9025fbaba4386

Branch pushed to git repo; I updated commit sha1. New commits:

86350b3Fix doctest formatting

comment:19 Changed 7 years ago by jdemeyer

  • 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 sage-preparse. 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 jsrn

  • Branch changed from u/jdemeyer/ticket/17019 to u/jsrn/ticket/17019

comment:21 follow-ups: Changed 7 years ago by jsrn

  • 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 doc-string).

Now for this ticket: the change in sage-preparse could be rolled back since it is not necessary for the moment. The doc-test 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 jdemeyer

Replying to jsrn:

Now for this ticket: the change in sage-preparse 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 jdemeyer

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 jsrn

  • Branch changed from u/jdemeyer/ticket/17019 to u/jsrn/ticket/17019

comment:25 Changed 7 years ago by jsrn

  • 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 bla-bla-bla" which appears twice above).

What do I do?


New commits:

6d8a330Put a constant in the test

comment:26 Changed 7 years ago by jsrn

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 git

  • Commit changed from 6d8a3303e88859a78a3809d7dc12bcff4306440f to f21f3862591b3de7968f8d552f641f02ab8f28fa

Branch pushed to git repo; I updated commit sha1. New commits:

baec18cMerge branch 'u/jsrn/ticket/17019' of git://trac.sagemath.org/sage into 17019_preparser_docstrings
f21f386upd fix date

comment:28 Changed 6 years ago by jsrn

  • Description modified (diff)
  • Status changed from needs_work to needs_review

I'm reviving this: Sage has displayed the doc-mangling 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 vbraun

  • 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 jsrn

Necro-review -- thanks! :-)

comment:31 Changed 5 years ago by vbraun

  • 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 chapoton

  • Authors changed from Johan S. R. Nielsen to Johan Sebastian Rosenkilde Nielsen
  • Commit f21f3862591b3de7968f8d552f641f02ab8f28fa deleted
Note: See TracTickets for help on using tickets.