Opened 7 years ago

Closed 7 years ago

#16992 closed enhancement (fixed)

Direct Fixdoctests

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

Status badges

Description (last modified by vbraun)

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 to src/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 7 years ago by vbraun

  • Branch set to u/vbraun/direct_fixdoctests

comment:2 Changed 7 years ago by vbraun

  • Authors set to Volker Braun
  • 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

New commits:

8c5f2b0Directly overwrite with fixed doctests

comment:3 Changed 7 years ago by jdemeyer

I would replace sage -git by git since this is supposed to be run from a Sage shell anyway.

comment:4 Changed 7 years ago by git

  • Commit changed from 8c5f2b0e915d525b7984b43721a869b6040ed20f to da59f20583c9a9aeb0ca64c9a1403da412666bd6

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

da59f20call git directly and from the right directory

comment:5 Changed 7 years ago by jdemeyer

  • Status changed from needs_review to needs_work

You also need to fix the documentation (output of ./sage --advanced)

comment:6 Changed 7 years ago by jdemeyer

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: 
    65156515    cdef double d = <double>q64
    65166516    if resultsign < 0:
    65176517        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 7 years ago by jdemeyer

Shouldn't

if relative.startswith('.'):

be

if relative.startswith('..'):

(although it seems that relpath strips ./)

comment:8 Changed 7 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer

There is of course a large margin for improvement in the sage-fixdoctests 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:9 Changed 7 years ago by vbraun

  • Status changed from needs_work to positive_review

done

comment:10 Changed 7 years ago by git

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

e397b3eAdd newline at end of file and fix cmdline help

comment:11 Changed 7 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:12 Changed 7 years ago by git

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

1b99c68Also fix the cmdline doctest

comment:13 Changed 7 years ago by vbraun

  • Status changed from needs_review to positive_review

Forgot the cmdline test...

comment:14 Changed 7 years ago by jdemeyer

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 7 years ago by vbraun

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 7 years ago by jdemeyer

lgtm (I was going to set positive_review but apparently you already did that :-p)

comment:17 Changed 7 years ago by vbraun

  • Branch changed from u/vbraun/direct_fixdoctests to 1b99c68fdfe02cbbbc57fe4dab882505a136d3e6
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.