Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#26263 closed defect (fixed)

py3: minor fixes for sage.rings.number_field

Reported by: embray Owned by:
Priority: minor Milestone: sage-8.4
Component: python3 Keywords:
Cc: chapoton Merged in:
Authors: Erik Bray Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: f5d4feb (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges


These are the remaining fixes to this package in my python3 branch that haven't been upstreamed yet.

Apparently I already had a hash implementation for the Order class, but this supplements the docstring with additional tests I had written.

Regarding the timestr() function, I would normally be wary of removing any non-underscored function without a deprecation, but in this case I believe it is justified:

  1. It was only ever used as a utility function in a different module that was removed 8 years ago (#9359), at which point it became dead code. It should have been removed with the rest of #9359.
  1. Nothing else uses or has ever used it.
  1. It doesn't have anything to do with the rest of the surrounding code; there's no reason anyone would look to that module for such a function.
  1. It is not well implemented even for what it's supposed to do. In the remote chance that someone is actually using this in their code they almost certainly have bigger problems ;)

Change History (4)

comment:1 Changed 4 years ago by embray

  • Cc chapoton added
  • Status changed from new to needs_review

comment:2 Changed 4 years ago by chapoton

  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

ok, let it be

comment:3 Changed 4 years ago by vbraun

  • Branch changed from u/embray/python3/sage-rings-number_field/misc to f5d4feb3f90d09e84f5ab4ed8fa8327460165206
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:4 Changed 4 years ago by embray

  • Commit f5d4feb3f90d09e84f5ab4ed8fa8327460165206 deleted

Ack, I missed a range(...) that needed updating, but was not covered by the tests in that module. I'll just include it with my next update...

Note: See TracTickets for help on using tickets.