Opened 7 years ago
Closed 7 years ago
#15757 closed enhancement (fixed)
Make sage.misc.superseded independent of sage.rings.integer_ring
Reported by:  nthiery  Owned by:  

Priority:  major  Milestone:  sage6.2 
Component:  misc  Keywords:  
Cc:  Merged in:  
Authors:  Nicolas M. Thiéry  Reviewers:  Nathann Cohen, Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  u/jdemeyer/ticket/15757 (Commits, GitHub, GitLab)  Commit:  86568171d749d4fbca7e6fb855fc162377f6cfec 
Dependencies:  Stopgaps: 
Description (last modified by )
This ticket removes the dependency of sage.misc.superseded upon sage.rings.integer_ring.is_IntegerRing which was used for type checking. This allows for using the functions in this module in code that is imported early upon Sage startup, in particular in category code.
By the way, this removes an unneeded test (the ticket number cannot be None).
Change History (22)
comment:1 Changed 7 years ago by
 Branch set to u/nthiery/ticket/15757
 Created changed from 01/29/14 06:48:06 to 01/29/14 06:48:06
 Modified changed from 01/29/14 06:48:06 to 01/29/14 06:48:06
comment:2 Changed 7 years ago by
 Description modified (diff)
 Status changed from new to needs_review
 Summary changed from Make sage.misc.superseded independent of sage.rings.integer_ring to Make sage.misc.superseded lazily import sage.rings.integer_ring
 Type changed from defect to enhancement
comment:3 Changed 7 years ago by
 Commit set to db4e2807f18e3b0e13c5101f4531483ba64b6f91
comment:4 Changed 7 years ago by
 Description modified (diff)
comment:5 Changed 7 years ago by
 Reviewers set to Nathann Cohen
 Status changed from needs_review to positive_review
Yo !
Looks good, no problem. All tests in combinat/ pass on my computer, and the patchbot will check the rest. This being said, you wrote your patch on top of 5.13.rc0
and we are on 6.1.rc0
at the moment ! Are you using 'develop' (and not 'master') ? This caused no conflict here.
Nathann
comment:6 Changed 7 years ago by
 Modified changed from 01/29/14 08:38:50 to 01/29/14 08:38:50
Thanks Nathann!
This being said, you wrote your patch on top of
5.13.rc0
and we are on6.1.rc0
at the moment ! Are you using 'develop' (and not 'master') ? This caused no conflict here.
Oops! I was using develop, but had apparently not pulled on that branch in a (very) long while. Thanks for the notice!
Cheers,
Nicolas
comment:7 followup: ↓ 13 Changed 7 years ago by
Why not remove _check_trac_number(trac_number)
altogether? Perhaps convert the number to int()
or use __index__()
, which will throw an exception if needed.
comment:8 Changed 7 years ago by
 Status changed from positive_review to needs_work
Yep. And as it looks like it is only used once, really ...
What is __index__()
though ? O_o
Nathann
comment:9 Changed 7 years ago by
Oh, it is used several times in the file, sorry. But yeah, looks like we do not need a function just to check that something is a positive integer :P
There will be false positives anyway as there is no ticket number 9999999999. Yet.
Nathann
comment:10 Changed 7 years ago by
__index__()
converts to a Python int
, but only if the object really is a kind of integer. x.__index__()
is a more strict version of int(x)
. For example::
sage: ZZ(1).__index__() 1 sage: RR(1).__index__()  AttributeError Traceback (most recent call last) <ipythoninput29d1cfb1a099d> in <module>() > 1 RR(Integer(1)).__index__() /usr/local/src/sagegit/local/lib/python2.7/sitepackages/sage/structure/element.so in sage.structure.element.Element.__getattr__ (sage/structure/element.c:3874)() /usr/local/src/sagegit/local/lib/python2.7/sitepackages/sage/structure/misc.so in sage.structure.misc.getattr_from_other_class (sage/structure/misc.c:1696)() AttributeError: 'sage.rings.real_mpfr.RealNumber' object has no attribute '__index__' sage: "1".__index__()  AttributeError Traceback (most recent call last) <ipythoninput48e5c2bcfbfe8> in <module>() > 1 "1".__index__() AttributeError: 'str' object has no attribute '__index__'
comment:11 Changed 7 years ago by
 Commit changed from db4e2807f18e3b0e13c5101f4531483ba64b6f91 to c57640dab99e12a8fa94acc49cb4ee478704fcfe
Branch pushed to git repo; I updated commit sha1. New commits:
c57640d  Completely got rid of the dependency of sage.misc.superseded upon is_Integer thanks to suggestion of Jeroen

comment:12 Changed 7 years ago by
 Description modified (diff)
 Modified changed from 01/29/14 20:41:30 to 01/29/14 20:41:30
 Reviewers changed from Nathann Cohen to Nathann Cohen, Jeroen Demeyer
 Status changed from needs_work to needs_review
comment:13 in reply to: ↑ 7 Changed 7 years ago by
Replying to jdemeyer:
Why not remove
_check_trac_number(trac_number)
altogether? Perhaps convert the number toint()
or use__index__()
, which will throw an exception if needed.
Ah, thanks for the pointer to index! That's exactly what I was looking for. In a first version, I was using int(), but then "10963" was accepted.
I left _check_trac_number
though, since someone had taken the
time to write a nice error message, and a .index AttributeError? is
not super informative in this case.
Oh, big improvement: now the number must be bigger than 1 :)
Cheers,
Nicolas
comment:14 Changed 7 years ago by
 Summary changed from Make sage.misc.superseded lazily import sage.rings.integer_ring to Make sage.misc.superseded independent of sage.rings.integer_ring
comment:15 followup: ↓ 17 Changed 7 years ago by
Added a small reviewer patch and merged sage6.1.rc0.
comment:16 Changed 7 years ago by
 Branch changed from u/nthiery/ticket/15757 to u/jdemeyer/ticket/15757
 Modified changed from 01/29/14 21:36:19 to 01/29/14 21:36:19
comment:17 in reply to: ↑ 15 Changed 7 years ago by
 Commit changed from c57640dab99e12a8fa94acc49cb4ee478704fcfe to 86568171d749d4fbca7e6fb855fc162377f6cfec
Replying to jdemeyer:
Added a small reviewer patch and merged sage6.1.rc0.
I just checked out your change, and am happy with them. Thanks!
So I guess that's a positive review assuming the patchpot goes green.
New commits:
e91ee5d  Merge remotetracking branch 'origin/develop' into ticket/15757

8656817  Don't use bare except; reviewer patch

comment:18 followup: ↓ 19 Changed 7 years ago by
We hit a race condition :) Unlike what the order above suggests, my comment applies to the latest commit.
Hmm, how to you like the term "patchpot" ?
comment:19 in reply to: ↑ 18 Changed 7 years ago by
Replying to nthiery:
We hit a race condition :)
Is it really a race condition? I think it's just a plain bug. Changing the branch doesn't automatically add a "new commits" message.
comment:20 Changed 7 years ago by
 Status changed from needs_review to positive_review
comment:21 Changed 7 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:22 Changed 7 years ago by
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
15757: make sage.misc.superseded import lazily sage.rings.integer_ring