Opened 8 years ago

Closed 6 years ago

#13255 closed enhancement (fixed)

Replace some deprecated python functions in sage/misc

Reported by: aapitzsch Owned by: jason
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) Commit: 0c7d2ff469ef65dde7b90e0b50f018bd30b8c3c0
Dependencies: Stopgaps:

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 7 years ago.
trac_13255_rebased.2.patch (51.7 KB) - added by aapitzsch 7 years ago.

Download all attachments as: .zip

Change History (36)

comment:1 Changed 8 years ago by aapitzsch

  • Status changed from new to needs_review

comment:2 Changed 8 years ago by aapitzsch

  • Dependencies set to #13109

comment:3 Changed 8 years ago by aapitzsch

  • Dependencies #13109 deleted
  • Description modified (diff)

Apply trac_13255_rebased.patch

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

comment:4 Changed 7 years ago by aapitzsch

Apply trac_13255_rebased.patch

comment:5 Changed 7 years ago by kcrisman

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 follow-up: Changed 7 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 7 years ago by 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.

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 7 years ago by kcrisman (previous) (diff)

Changed 7 years ago by aapitzsch

comment:8 Changed 7 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 7 years ago by ncohen

  • Status changed from needs_review to needs_work

Does not apply on 5.11.beta3 !

Nathann

Changed 7 years ago by aapitzsch

comment:10 Changed 7 years ago by aapitzsch

  • Description modified (diff)
  • Status changed from needs_work to needs_review

Apply trac_13255_rebased.2.patch

comment:11 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:12 Changed 6 years ago by aapitzsch

  • Branch set to u/aapitzsch/ticket/13255
  • Commit set to 8c69c8c16615a2e67e4e133c799b84caa24bca3e
  • Description modified (diff)

New commits:

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

comment:13 Changed 6 years ago by ncohen

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

Nathann

comment:14 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:15 Changed 6 years ago by git

  • Commit changed from 8c69c8c16615a2e67e4e133c799b84caa24bca3e to 793102a47f8bdcf38791421f6bc82da9a93403f9

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

793102aMerge branch 'develop' into py3_misc

comment:16 Changed 6 years ago by git

  • Commit changed from 793102a47f8bdcf38791421f6bc82da9a93403f9 to 1c7f78f449395a9187bb10932fbfb231d7af24cb

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

1c7f78fMerge branch 'develop' into py3_misc

comment:17 Changed 6 years ago by git

  • Commit changed from 1c7f78f449395a9187bb10932fbfb231d7af24cb to b86b57b43cf15355f020174842aaf8e4c6fe5ad4

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

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

comment:18 follow-up: Changed 6 years ago by 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?

comment:19 Changed 6 years ago by chapoton

  • Status changed from needs_review to needs_work

does not apply

comment:20 Changed 6 years ago by git

  • Commit changed from b86b57b43cf15355f020174842aaf8e4c6fe5ad4 to 69a0a48bf8e643c8fed6cb8ed4bbd7540e5f60b3

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

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

comment:21 Changed 6 years ago by git

  • Commit changed from 69a0a48bf8e643c8fed6cb8ed4bbd7540e5f60b3 to ecc62ecd400221f880903dce69187b51994d01b7

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 6 years ago by aapitzsch

  • Status changed from needs_work to needs_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 follow-up: Changed 6 years ago by wluebbe

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 6 years ago by wluebbe (previous) (diff)

comment:24 Changed 6 years ago by git

  • Commit changed from ecc62ecd400221f880903dce69187b51994d01b7 to 8c5c945e19b82a71e90a8d56d372e075aea8b8fc

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 6 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 6 years ago by wluebbe

  • Status changed from needs_review to positive_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 6 years ago by vbraun

reviewer name

comment:28 Changed 6 years ago by wluebbe

  • Reviewers set to Wilfried Luebbe

comment:29 Changed 6 years ago by vbraun

Conflicts, mostly with #15990

comment:30 Changed 6 years ago by wluebbe

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

comment:31 Changed 6 years ago by git

  • Commit changed from 8c5c945e19b82a71e90a8d56d372e075aea8b8fc to b7a33ec7d9db24da1edfc98617e4b03f06f2f632
  • Status changed from positive_review to needs_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 6 years ago by wluebbe

  • Branch changed from u/aapitzsch/ticket/13255 to u/wluebbe/ticket/13255
  • Commit changed from b7a33ec7d9db24da1edfc98617e4b03f06f2f632 to 0c7d2ff469ef65dde7b90e0b50f018bd30b8c3c0

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 6 years ago by aapitzsch

  • Status changed from needs_review to positive_review

comment:34 Changed 6 years ago by vbraun

  • Branch changed from u/wluebbe/ticket/13255 to 0c7d2ff469ef65dde7b90e0b50f018bd30b8c3c0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.