Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace some deprecated python functions in sage/algebras #12461

Closed
a-andre opened this issue Feb 7, 2012 · 18 comments
Closed

Replace some deprecated python functions in sage/algebras #12461

a-andre opened this issue Feb 7, 2012 · 18 comments

Comments

@a-andre
Copy link

a-andre commented Feb 7, 2012

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 attachment: trac_12461.patch and attachment: trac_12461_2b.patch to the Sage library repository.

Depends on #12484

Component: misc

Author: André Apitzsch

Reviewer: David Loeffler

Merged: sage-5.0.beta11

Issue created by migration from https://trac.sagemath.org/ticket/12461

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Mar 12, 2012

comment:2

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

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Mar 12, 2012

Work Issues: needs rebase

@loefflerd loefflerd mannequin added s: needs work and removed s: needs review labels Mar 12, 2012
@a-andre
Copy link
Author

a-andre commented Mar 12, 2012

Attachment: trac_12461.patch.gz

@a-andre
Copy link
Author

a-andre commented Mar 12, 2012

comment:3

Rebased.

@a-andre
Copy link
Author

a-andre commented Mar 12, 2012

Changed work issues from needs rebase to none

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Mar 12, 2012

comment:4

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

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Mar 12, 2012

Dependencies: #12484

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Mar 12, 2012

comment:5

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.

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Mar 12, 2012

Reviewer: David Loeffler

@loefflerd loefflerd mannequin added s: needs work and removed s: needs review labels Mar 12, 2012
@a-andre
Copy link
Author

a-andre commented Mar 12, 2012

Attachment: trac_12461_2.patch.gz

Apply after trac_12461.patch

@a-andre
Copy link
Author

a-andre commented Mar 12, 2012

comment:6

Replying to @loefflerd:

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.

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Mar 12, 2012

comment:7

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.

@loefflerd loefflerd mannequin added s: needs work and removed s: needs review labels Mar 12, 2012
@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Mar 12, 2012

apply over trac_12461.patch

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Mar 12, 2012

comment:8

Attachment: trac_12461_2b.patch.gz

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.

@loefflerd loefflerd mannequin removed the s: needs work label Mar 12, 2012
@jdemeyer
Copy link

comment:10

Please clarify which patches have to be added.

@a-andre
Copy link
Author

a-andre commented Mar 13, 2012

comment:11

Add

  1. trac_12461.patch
  2. trac_12461_2b.patch

@jhpalmieri

This comment has been minimized.

@jdemeyer
Copy link

Merged: sage-5.0.beta11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants