Opened 6 years ago
Closed 6 years ago
#16992 closed enhancement (fixed)
Direct Fixdoctests
Reported by:  vbraun  Owned by:  

Priority:  major  Milestone:  sage6.4 
Component:  scripts  Keywords:  
Cc:  Merged in:  
Authors:  Volker Braun  Reviewers:  Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  1b99c68 (Commits, GitHub, GitLab)  Commit:  1b99c68fdfe02cbbbc57fe4dab882505a136d3e6 
Dependencies:  Stopgaps: 
Description (last modified by )
Now that we are actually using a versions control system as it is intended, the sage fixdoctests
workflow is a bit antiquated:
 Run
sage fixdoctests src/sage/foo.py
 Get a diff printed to stdout
 copy
src/sage/foo.py.out
tosrc/sage/foo.py
 Run
git diff
to get the same diff again
Instead, just overwrite the file directly. Use version control for the rest.
Change History (17)
comment:1 Changed 6 years ago by
 Branch set to u/vbraun/direct_fixdoctests
comment:2 Changed 6 years ago by
 Commit set to 8c5f2b0e915d525b7984b43721a869b6040ed20f
 Component changed from PLEASE CHANGE to scripts
 Description modified (diff)
 Status changed from new to needs_review
 Type changed from PLEASE CHANGE to enhancement
comment:3 Changed 6 years ago by
I would replace sage git
by git
since this is supposed to be run from a Sage shell anyway.
comment:4 Changed 6 years ago by
 Commit changed from 8c5f2b0e915d525b7984b43721a869b6040ed20f to da59f20583c9a9aeb0ca64c9a1403da412666bd6
Branch pushed to git repo; I updated commit sha1. New commits:
da59f20  call git directly and from the right directory

comment:5 Changed 6 years ago by
 Status changed from needs_review to needs_work
You also need to fix the documentation (output of ./sage advanced
)
comment:6 Changed 6 years ago by
And could you please add a \n
to the output file, because otherwise you always get

src/sage/rings/integer.pyx
diff git a/src/sage/rings/integer.pyx b/src/sage/rings/integer.pyx index c85ba09..59cb6c1 100644
a b cdef double mpz_get_d_nearest(mpz_t x) except? 648555075988944.5: 6515 6515 cdef double d = <double>q64 6516 6516 if resultsign < 0: 6517 6517 d = d 6518 return ldexp(d, shift) 6518 return ldexp(d, shift) 6519 No newline at end of file
(I know you didn't introduce this bug, but it's such a trivial fix...)
comment:7 Changed 6 years ago by
Shouldn't
if relative.startswith('.'):
be
if relative.startswith('..'):
(although it seems that relpath
strips ./
)
comment:8 Changed 6 years ago by
 Reviewers set to Jeroen Demeyer
There is of course a large margin for improvement in the sagefixdoctests
script (ideally, it should be integrated within the doctesting framework), but if you make the small changes I mentioned, I'm happy with this ticket.
comment:10 Changed 6 years ago by
 Commit changed from da59f20583c9a9aeb0ca64c9a1403da412666bd6 to e397b3e6b1dfa0c0a7877adb89efa278495000f9
 Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
e397b3e  Add newline at end of file and fix cmdline help

comment:11 Changed 6 years ago by
 Status changed from needs_review to positive_review
comment:12 Changed 6 years ago by
 Commit changed from e397b3e6b1dfa0c0a7877adb89efa278495000f9 to 1b99c68fdfe02cbbbc57fe4dab882505a136d3e6
 Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
1b99c68  Also fix the cmdline doctest

comment:13 Changed 6 years ago by
 Status changed from needs_review to positive_review
Forgot the cmdline test...
comment:14 Changed 6 years ago by
Any reason you write
with open(test_file, 'r') as f: fixed_test = f.read()
instead of
fixed_test = open(test_file).read()
comment:15 Changed 6 years ago by
I don't really care either way much, but to me the first version makes it clearer how long the file is actually open for.
comment:16 Changed 6 years ago by
lgtm (I was going to set positive_review but apparently you already did that :p)
comment:17 Changed 6 years ago by
 Branch changed from u/vbraun/direct_fixdoctests to 1b99c68fdfe02cbbbc57fe4dab882505a136d3e6
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Directly overwrite with fixed doctests