Opened 8 years ago

Closed 8 years ago

#14609 closed enhancement (fixed)

cleanup of doc in ell_point.py

Reported by: chapoton Owned by: cremona
Priority: minor Milestone: sage-5.11
Component: elliptic curves Keywords: documentation, elliptic curves
Cc: Merged in: sage-5.11.beta1
Authors: Frédéric Chapoton Reviewers: John Cremona
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #13953, #13213 Stopgaps:

Description (last modified by chapoton)

this ticket aims to clean up the doc (and code) of ell_point.py

(using the reports of pyflakes and pep8)

  • add necessary imports, removed unused ones
  • removes trailing whitespaces
  • change raise syntax to python3
  • put code into pep8 shape (spaces, etc)
  • links to wikipedia and trac
  • correct reference links

Attachments (1)

trac_14609_ell_point_cleanup.patch (85.6 KB) - added by chapoton 8 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 8 years ago by cremona

I have had a look through the patch. Am I right that almost all the changes are either (1) tidying up whitespace, (2) changing syntax for error raising or (3) small changes to docstring formatting? I saw almost nothing else.

There are a few places where the syntax of a trac reference is changed but still does not use the :trac:1234 form. Is that an oversight?

Otherwise, assuming that all the doctests still pass I see no reason why not to approve this patch, though I am slightly nervous about conflicts with other patches which affect this file. Specifically, does this apply after #12509?

comment:2 follow-up: Changed 8 years ago by chapoton

  • Authors set to Frédéric Chapoton
  • Description modified (diff)
  • Status changed from new to needs_review

Ok, here is a new patch, which applies on 5.10.beta3.

I have taken care of the forgotten trac links.

Yes, this patch only does trivial things, but I understand your concerns. I should not have broken anything, I hope I was careful enough.

By the way, do you know of other coming patches touching this file ?

comment:3 in reply to: ↑ 2 Changed 8 years ago by cremona

  • Reviewers set to John Cremona
  • Status changed from needs_review to positive_review

Replying to chapoton:

Ok, here is a new patch, which applies on 5.10.beta3.

I have taken care of the forgotten trac links.

Yes, this patch only does trivial things, but I understand your concerns. I should not have broken anything, I hope I was careful enough.

By the way, do you know of other coming patches touching this file ?

No -- but of course there might be, we will see.

This patch applies fine to 5.10.beta3, the tests in the touched file all pass, and sage -docbuild reference html works fine (though it did say that no targets were out of date in [schemes] which seems odd. Nevertheless I think this deserves a positive review as we do not want more delay or it may need rebasing again.

comment:4 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work
  • Work issues set to rebase

This doesn't apply to sage-5.10.beta4:

patching file sage/schemes/elliptic_curves/ell_point.py
Hunk #100 FAILED at 2677
Hunk #102 succeeded at 2795 with fuzz 2 (offset 6 lines).
Hunk #103 FAILED at 2797
Hunk #104 FAILED at 2820
3 out of 127 hunks FAILED -- saving rejects to file sage/schemes/elliptic_curves/ell_point.py.rej

comment:5 Changed 8 years ago by jdemeyer

  • Dependencies set to #13953, #13213

comment:6 Changed 8 years ago by cremona

Sorry Frederic, as I feared there were some new dependencies and beta4 came out after I had done my test.

comment:7 Changed 8 years ago by chapoton

ok, I will take care of that, but not right now.

Changed 8 years ago by chapoton

comment:8 Changed 8 years ago by chapoton

  • Status changed from needs_work to needs_review

patch has been rebased

comment:9 follow-up: Changed 8 years ago by chapoton

This should be good to go. Anybody for a review ?

comment:10 in reply to: ↑ 9 ; follow-up: Changed 8 years ago by cremona

  • Status changed from needs_review to positive_review

Replying to chapoton:

This should be good to go. Anybody for a review ?

I checked that the patch applies fine to 5.10.rc0, and did a complete test, hence another positive review. Very sorry my delay caused this to miss 5.10 (unless there is to be another rc version -- this patch only affects docstrings!)

comment:11 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.10 to sage-5.11
  • Work issues rebase deleted

comment:12 in reply to: ↑ 10 ; follow-up: Changed 8 years ago by jdemeyer

Replying to cremona:

unless there is to be another rc version

Yes, there will be, but since this is a large and unimportant patch, I would rather postpone it.

this patch only affects docstrings!

Looking at the patch, that's actually far from true...

comment:13 in reply to: ↑ 12 Changed 8 years ago by cremona

Replying to jdemeyer:

Replying to cremona:

unless there is to be another rc version

Yes, there will be, but since this is a large and unimportant patch, I would rather postpone it.

No problem.

this patch only affects docstrings!

Looking at the patch, that's actually far from true...

OK I admit that (it's not my patch) -- docstrings, whilespace, and some pyflakes/pep8 tidying. So, not just docstrings.

comment:14 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.11.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.