Make sure preparser for .sage files respects module docstrings

[johanrosenkilde]

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

Component: misc

Keywords: preparse

Author: Johan Sebastian Rosenkilde Nielsen

Branch: f21f386

Reviewer: Jeroen Demeyer, Volker Braun

Issue created by migration from https://trac.sagemath.org/ticket/17019

[johanrosenkilde]

Branch: u/jsrn/preparser_for__sage_files_no_longer_respects_module_docstrings

[johanrosenkilde]

Commit: 9f82a15

[johanrosenkilde]

comment:2

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:

f9f9727	Simple change to sage-preparse to make sure the module docstring is put first
9f82a15	Merge w. atomic_write change from 6.3

[jdemeyer]

comment:3

Replying to @johanrosenkilde:

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

[jdemeyer]

comment:4

Also, you don't need an additional

from sage.misc.preparser import preparse_file

[sagetrac-git]

Changed commit from 9f82a15 to df1d574

[sagetrac-git]

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

df1d574

Added doctest. Rm unneccessary import line

[johanrosenkilde]

comment:6

Ok, I've added a doctest for this.

[johanrosenkilde]

comment:7

The Build-bot's doctests failing is in some numerical stability tests, and seem to have nothing to do with this patch.

[jdemeyer]

comment:8

Sorry to bother again, but you should rebase this over #16955.

[jdemeyer]

comment:9

...and instead of adding a completely new doctest, why not simply change the old test for sage --preparse?

[jdemeyer]

comment:10

You removed support for coding.

[sagetrac-git]

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

[sagetrac-git]

Changed commit from df1d574 to bedc186

[johanrosenkilde]

comment:12

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.

[jdemeyer]

comment:13

Replying to @johanrosenkilde:

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.

[sagetrac-git]

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

88eee97

Write out the coding line

[sagetrac-git]

Changed commit from bedc186 to 88eee97

[sagetrac-git]

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

[johanrosenkilde]

Changed branch from u/jsrn/ticket/17019 to u/jdemeyer/ticket/17019

[jdemeyer]

comment:22

Replying to @johanrosenkilde:

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.

[jdemeyer]

comment:23

Replying to @johanrosenkilde:

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.

[johanrosenkilde]

Changed branch from u/jdemeyer/ticket/17019 to u/jsrn/ticket/17019

[johanrosenkilde]

Changed commit from 86350b3 to 6d8a330

[johanrosenkilde]

comment:25

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:

6d8a330

Put a constant in the test

[johanrosenkilde]

comment:26

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?

[sagetrac-git]

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

baec18c	Merge branch 'u/jsrn/ticket/17019' of git://trac.sagemath.org/sage into 17019_preparser_docstrings
f21f386	upd fix date

[sagetrac-git]

Changed commit from 6d8a330 to f21f386

[johanrosenkilde]

comment:28

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.

[vbraun]

Changed reviewer from Jeroen Demeyer to Jeroen Demeyer, Volker Braun

[johanrosenkilde]

comment:30

Necro-review -- thanks! :-)

[vbraun]

Changed branch from u/jsrn/ticket/17019 to f21f386

[fchapoton]

Changed commit from f21f386 to none

[fchapoton]

Changed author from Johan S. R. Nielsen to Johan Sebastian Rosenkilde Nielsen