Opened 10 years ago

Closed 9 years ago

#13255 closed enhancement (fixed)

Replace some deprecated python functions in sage/misc

Reported by: aapitzsch Owned by: Jason Grout
Priority: major Milestone: sage-6.2
Component: misc Keywords: python3
Cc: Merged in:
Authors: André Apitzsch Reviewers: Wilfried Luebbe
Report Upstream: N/A Work issues:
Branch: 0c7d2ff (Commits, GitHub, GitLab) Commit: 0c7d2ff469ef65dde7b90e0b50f018bd30b8c3c0
Dependencies: Stopgaps:

Status badges

Description (last modified by aapitzsch)

Moving forward to Python 3.

Execute

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

for each *.py (and *.pyx) in sage/misc.

Attachments (2)

trac_13255_rebased.patch (56.0 KB) - added by aapitzsch 10 years ago.
trac_13255_rebased.2.patch (51.7 KB) - added by aapitzsch 9 years ago.

Download all attachments as: .zip

Change History (36)

comment:1 Changed 10 years ago by aapitzsch

Status: newneeds_review

comment:2 Changed 10 years ago by aapitzsch

Dependencies: #13109

comment:3 Changed 10 years ago by aapitzsch

Dependencies: #13109
Description: modified (diff)

Apply trac_13255_rebased.patch

Last edited 10 years ago by aapitzsch (previous) (diff)

comment:4 Changed 10 years ago by aapitzsch

Apply trac_13255_rebased.patch

comment:5 Changed 10 years ago by Karl-Dieter Crisman

Two questions that sound overly critical, but are really just out of curiosity:

  • Curious why you ported these forward right now; the current release manager has been skeptical of changes hitting many files that are mainly cosmetic (like whitespace), since it means a lot of rebasing for others (or can, anyway) - not that these won't be very needed eventually, but we aren't switching to Python 3 anytime soon.
  • Why not also move to the new string formatting syntax for these (ones only)? The "percent" style wouldn't work in Python 3 either, so the job is sort of half done.

comment:6 Changed 10 years ago by aapitzsch

Replying to kcrisman:

  • Curious why you ported these forward right now; the current release manager has been skeptical of changes hitting many files that are mainly cosmetic (like whitespace), since it means a lot of rebasing for others (or can, anyway) - not that these won't be very needed eventually, but we aren't switching to Python 3 anytime soon.

Although we aren't switching to Python 3 in the near future, this work has to be done anyway. It might also speed up the process.

  • Why not also move to the new string formatting syntax for these (ones only)? The "percent" style wouldn't work in Python 3 either, so the job is sort of half done.

The "percent" style still works in Python 3 and I couldn't find a notice about its deprecation.

Thanks for reviewing my patches.

comment:7 in reply to:  6 Changed 10 years ago by Karl-Dieter Crisman

  • Curious why you ported these forward right now; the current release manager has been skeptical of changes hitting many files that are mainly cosmetic (like whitespace), since it means a lot of rebasing for others (or can, anyway) - not that these won't be very needed eventually, but we aren't switching to Python 3 anytime soon.

Although we aren't switching to Python 3 in the near future, this work has to be done anyway. It might also speed up the process.

See the current discussion at #13899 for why it might not be wise to do a large, commonly used directory all at once.

  • Why not also move to the new string formatting syntax for these (ones only)? The "percent" style wouldn't work in Python 3 either, so the job is sort of half done.

The "percent" style still works in Python 3 and I couldn't find a notice about its deprecation.

Interesting but true! My usual source of info apparently never was incorporated. My guess is that so many people found it irksome to switch legacy code that they never bothered deprecating it ... yet.

Last edited 10 years ago by Karl-Dieter Crisman (previous) (diff)

Changed 10 years ago by aapitzsch

Attachment: trac_13255_rebased.patch added

comment:8 Changed 10 years ago by aapitzsch

It seems that patchbot tries to apply every patch attached to one ticket if the uploaded patch overwrites another with the same name although a previous comment said to apply only selected ones.

Apply trac_13255_rebased.patch.

comment:9 Changed 9 years ago by Nathann Cohen

Status: needs_reviewneeds_work

Does not apply on 5.11.beta3 !

Nathann

Changed 9 years ago by aapitzsch

Attachment: trac_13255_rebased.2.patch added

comment:10 Changed 9 years ago by aapitzsch

Description: modified (diff)
Status: needs_workneeds_review

Apply trac_13255_rebased.2.patch

comment:11 Changed 9 years ago by Jeroen Demeyer

Milestone: sage-5.11sage-5.12

comment:12 Changed 9 years ago by aapitzsch

Branch: u/aapitzsch/ticket/13255
Commit: 8c69c8c16615a2e67e4e133c799b84caa24bca3e
Description: modified (diff)

New commits:

8c69c8ctrac 13255: replace some deprecated python functions in sage/misc

comment:13 Changed 9 years ago by Nathann Cohen

I personally oppose the changes to the "print" function :-P

Nathann

comment:14 Changed 9 years ago by For batch modifications

Milestone: sage-6.1sage-6.2

comment:15 Changed 9 years ago by git

Commit: 8c69c8c16615a2e67e4e133c799b84caa24bca3e793102a47f8bdcf38791421f6bc82da9a93403f9

Branch pushed to git repo; I updated commit sha1. New commits:

793102aMerge branch 'develop' into py3_misc

comment:16 Changed 9 years ago by git

Commit: 793102a47f8bdcf38791421f6bc82da9a93403f91c7f78f449395a9187bb10932fbfb231d7af24cb

Branch pushed to git repo; I updated commit sha1. New commits:

1c7f78fMerge branch 'develop' into py3_misc

comment:17 Changed 9 years ago by git

Commit: 1c7f78f449395a9187bb10932fbfb231d7af24cbb86b57b43cf15355f020174842aaf8e4c6fe5ad4

Branch pushed to git repo; I updated commit sha1. New commits:

b86b57bMerge remote-tracking branch 'origin/develop' into py3_misc

comment:18 Changed 9 years ago by Wilfried Luebbe

I had a look at the patch "trac 13255: replace some deprecated python functions in sage/misc".

There are about 51 affected modules. 4 of them have a new line from __future__ import print_function.

But there are also modules with the new function print() but no from __future__. An example is /src/sage/misc/attached_files.py.

How can this work?

comment:19 Changed 9 years ago by Frédéric Chapoton

Status: needs_reviewneeds_work

does not apply

comment:20 Changed 9 years ago by git

Commit: b86b57b43cf15355f020174842aaf8e4c6fe5ad469a0a48bf8e643c8fed6cb8ed4bbd7540e5f60b3

Branch pushed to git repo; I updated commit sha1. New commits:

69a0a48Merge remote-tracking branch 'origin/develop' into py3_misc

comment:21 Changed 9 years ago by git

Commit: 69a0a48bf8e643c8fed6cb8ed4bbd7540e5f60b3ecc62ecd400221f880903dce69187b51994d01b7

Branch pushed to git repo; I updated commit sha1. New commits:

ecc62ecMerge remote-tracking branch 'origin/develop' into py3_misc

comment:22 in reply to:  18 Changed 9 years ago by aapitzsch

Status: needs_workneeds_review

Replying to wluebbe:

I had a look at the patch "trac 13255: replace some deprecated python functions in sage/misc".

There are about 51 affected modules. 4 of them have a new line from __future__ import print_function.

But there are also modules with the new function print() but no from __future__. An example is /src/sage/misc/attached_files.py.

How can this work?

In these cases it's still the print statement and not the function that is used. The parentheses are ignored like in

sage: a = (5)
sage: a
5

so in most cases you can write print("some string") instead of print "some string" without using from __future__

comment:23 Changed 9 years ago by Wilfried Luebbe

Yes, meanwhile I have also discovered it while working on ticket:15989.

But apparently the tool 2to3 fix_print (or futurize fix_print_with_import) does not detect this case and wraps another pair of parenthesis around it.

Maybe we should not add from __future__ import print_function but just make sure that the print statement has those extra parenthesis?

I will mention this in ticket:15989.

Last edited 9 years ago by Wilfried Luebbe (previous) (diff)

comment:24 Changed 9 years ago by git

Commit: ecc62ecd400221f880903dce69187b51994d01b78c5c945e19b82a71e90a8d56d372e075aea8b8fc

Branch pushed to git repo; I updated commit sha1. New commits:

8c5c945Merge remote-tracking branch 'origin/develop' into py3_misc

comment:25 in reply to:  23 Changed 9 years ago by aapitzsch

Replying to wluebbe:

Maybe we should not add from __future__ import print_function but just make sure that the print statement has those extra parenthesis?

In my opinion we should use from __future__ import print_function if necessary. It's a simple way to get python 2 and python 3 compatible print.

BTW could you review this patch? Thanks.

comment:26 Changed 9 years ago by Wilfried Luebbe

Status: needs_reviewpositive_review

Looks good to and all doctests pass.

from __future__ import print_function is apparently only added when trailing "," are converted to end=" ".

Currently I would suggest NOT to wrap the print parameters with parenthesis in doctests as this might be handled be a new version of the preparser (see also ticket:15989).

BTW: I hope the changes to the raise statements will not interfere to much with ticket:15990.

comment:27 Changed 9 years ago by Volker Braun

reviewer name

comment:28 Changed 9 years ago by Wilfried Luebbe

Reviewers: Wilfried Luebbe

comment:29 Changed 9 years ago by Volker Braun

Conflicts, mostly with #15990

comment:30 Changed 9 years ago by Wilfried Luebbe

Not really unexpected. We'll should try with next beta.

comment:31 Changed 9 years ago by git

Commit: 8c5c945e19b82a71e90a8d56d372e075aea8b8fcb7a33ec7d9db24da1edfc98617e4b03f06f2f632
Status: positive_reviewneeds_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

b7a33ecMerge remote-tracking branch 'origin/develop' into py3_misc

comment:32 Changed 9 years ago by Wilfried Luebbe

Branch: u/aapitzsch/ticket/13255u/wluebbe/ticket/13255
Commit: b7a33ec7d9db24da1edfc98617e4b03f06f2f6320c7d2ff469ef65dde7b90e0b50f018bd30b8c3c0

I tested the change OK.

While I went through the patch I did some polishing and added it to a reviewer patch.
If you agree with my branch, you can set this to positive review. Otherwise set your latest patch to positive review.

The majority of changes simplify "{0}{1}{2}" to "{}{}{}". Since Python 2.7 (and Python 3.1) the positional argument specifiers can be omitted Format strings.


New commits:

0c7d2ffreviewer patch

comment:33 Changed 9 years ago by aapitzsch

Status: needs_reviewpositive_review

comment:34 Changed 9 years ago by Volker Braun

Branch: u/wluebbe/ticket/132550c7d2ff469ef65dde7b90e0b50f018bd30b8c3c0
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.