Opened 7 years ago

Closed 7 years ago

#12461 closed enhancement (fixed)

Replace some deprecated python functions in sage/algebras

Reported by: aapitzsch Owned by: jason
Priority: major Milestone: sage-5.0
Component: misc Keywords:
Cc: Merged in: sage-5.0.beta11
Authors: André Apitzsch Reviewers: David Loeffler
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #12484 Stopgaps:

Description (last modified by jhpalmieri)

We should prepare moving to Python3.

Execute

2to3 -f has_key -f except -f idioms -f ne -f raise

for each *.py in sage/algebras.

See http://docs.python.org/library/2to3.html#fixers for meaning of the parameters.


Apply trac_12461.patch and trac_12461_2b.patch to the Sage library repository.

Attachments (3)

trac_12461.patch (66.2 KB) - added by aapitzsch 7 years ago.
trac_12461_2.patch (1007 bytes) - added by aapitzsch 7 years ago.
Apply after trac_12461.patch
trac_12461_2b.patch (751 bytes) - added by davidloeffler 7 years ago.
apply over trac_12461.patch

Download all attachments as: .zip

Change History (16)

comment:1 Changed 7 years ago by aapitzsch

  • Status changed from new to needs_review

comment:2 Changed 7 years ago by davidloeffler

  • Status changed from needs_review to needs_work
  • Work issues set to needs rebase

This conflicts with #12484 (merged in 5.0.beta5) -- can you rebase it?

Changed 7 years ago by aapitzsch

comment:3 Changed 7 years ago by aapitzsch

  • Status changed from needs_work to needs_review
  • Work issues needs rebase deleted

Rebased.

comment:4 Changed 7 years ago by davidloeffler

  • Dependencies set to #12484

I've added #12484 as a dependency (since there is a copy of the patchbot still running against 4.8)

comment:5 follow-up: Changed 7 years ago by davidloeffler

  • Reviewers set to David Loeffler
  • Status changed from needs_review to needs_work

I don't like the change at line 148 of free_algebra_quotient:

 	148	        return isinstance(self, type(right)) and \ 
149	149	               self.ngens() == right.ngens() and \ 
150	150	               self.rank() == right.rank() and \ 
151	151	               self.module() == right.module() and \ 

If type(right) was some very generic base class like SageObject, then the first statement would be True, but the remaining ones would be completely meaningless, so you'd get weird errors being raised. It should probably be isinstance(other, FreeAlgebraQuotient) or something like that.

That is my only disagreement with this patch, so if you can do a tiny fix for that I'll happily give it a positive review.

Changed 7 years ago by aapitzsch

Apply after trac_12461.patch

comment:6 in reply to: ↑ 5 Changed 7 years ago by aapitzsch

  • Status changed from needs_work to needs_review

Replying to davidloeffler:

If type(right) was some very generic base class like SageObject, then the first statement would be True, but the remaining ones would be completely meaningless, so you'd get weird errors being raised. It should probably be isinstance(other, FreeAlgebraQuotient) or something like that.

You are right. I couldn't think of another possible type, so I used FreeAlgebraQuotient.

comment:7 Changed 7 years ago by davidloeffler

  • Status changed from needs_review to needs_work

The double colon after EXAMPLES that your new patch adds shouldn't be there, and it causes an error in building the docs:

/storage/masiao/sage-5.0.beta7-patchbot/local/lib/python2.7/site-packages/sage/algebras/free_algebra_quotient.py:docstring of sage.algebras.free_algebra_quotient.FreeAlgebraQuotient:10: WARNING: Literal block expected; none found.

Changed 7 years ago by davidloeffler

apply over trac_12461.patch

comment:8 Changed 7 years ago by davidloeffler

  • Status changed from needs_work to needs_review

Here's a strict subset of your patch (literally obtained by deleting one of the two hunks from your patch in a text editor). If you're happy with that, you can set this to positive review.

comment:9 Changed 7 years ago by aapitzsch

  • Status changed from needs_review to positive_review

comment:10 Changed 7 years ago by jdemeyer

Please clarify which patches have to be added.

comment:11 Changed 7 years ago by aapitzsch

Add

  1. trac_12461.patch
  2. trac_12461_2b.patch

comment:12 Changed 7 years ago by jhpalmieri

  • Description modified (diff)

comment:13 Changed 7 years ago by jdemeyer

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