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

Python 3 preparation: Change some code to use more modern Python idioms #15984

Closed
wluebbe mannequin opened this issue Mar 20, 2014 · 14 comments
Closed

Python 3 preparation: Change some code to use more modern Python idioms #15984

wluebbe mannequin opened this issue Mar 20, 2014 · 14 comments

Comments

@wluebbe
Copy link
Mannequin

wluebbe mannequin commented Mar 20, 2014

This fixer changes code like type(x) == T) and while 1: and it introduces the sorted() function where appropriate.

These changes are optional, but they improve style and may provide a small performance gain.

Changes according to lib2to3/fixes/fix_idioms.py:

* Change some type comparisons to isinstance() calls:
    type(x) == T -> isinstance(x, T)
    type(x) is T -> isinstance(x, T)
    type(x) != T -> not isinstance(x, T)
    type(x) is not T -> not isinstance(x, T)

* Change "while 1:" into "while True:".

* Change both
    v = list(EXPR)
    v.sort()
    foo(v)

and the more general
    v = EXPR
    v.sort()
    foo(v)

into
    v = sorted(EXPR)
    foo(v)

Component: distribution

Author: Wilfried Luebbe

Branch/Commit: 6964533

Reviewer: Travis Scrimshaw

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

@wluebbe wluebbe mannequin added this to the sage-6.2 milestone Mar 20, 2014
@wluebbe wluebbe mannequin added c: distribution labels Mar 20, 2014
@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Mar 28, 2014

Branch: u/wluebbe/ticket/15984

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Mar 28, 2014

comment:1
  • At first I applied the 2to3 tool (idioms) -> first commit
  • Fixing the doctests failures of the first commit -> second commit. All tests passed.
  • Changed while 1: into while True: for cython modules -> third commit.

New commits:

86de429changes made by 2to3 tool (idioms), many changes
1f05981reverted changes to 4 modules done by 2to3 tool
85f7d5echange "while 1:" into "while True:" for .pyx modules

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Mar 28, 2014

Commit: 85f7d5e

@wluebbe wluebbe mannequin added the s: needs review label Mar 28, 2014
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 3, 2014

Changed commit from 85f7d5e to 6964533

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 3, 2014

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

617b158changes made by 2to3 tool (idioms), many changes
85ef438reverted changes to 4 modules done by 2to3 tool
6964533change "while 1:" into "while True:" for .pyx modules

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Apr 3, 2014

comment:3

Rebased on 6.2.beta6 and resolved merge conflicts.

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Apr 4, 2014

comment:4

And the test report:

All tests passed!
----------------------------------------------------------------------
Total time for all tests: 3214.7 seconds

@tscrim
Copy link
Collaborator

tscrim commented Apr 6, 2014

comment:5

I think it would also be good to change x == None to x is None while we're at it.

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Apr 6, 2014

comment:6

There are 151 modules (py and pyx combined) which have at least one of == None or != None.

The change makes sense as (by far) most modules use is None or is not None.

But as this change is already pretty large I would prefer to see this change in a separate ticket/branch.

@tscrim
Copy link
Collaborator

tscrim commented Apr 7, 2014

comment:7

I think it fits well with this ticket (plus how many of those modules overlap with this one?). Also one large sweeping change is the same to me as two large sweeping changes (especially with different commits), but with the added benefit of an only one time potential rebase. If you still disagree and want it in a separate ticket, then you can set this to positive review.

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Apr 8, 2014

Reviewer: Travis Scrimshaw

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Apr 8, 2014

comment:8

I generated 151 patched py and pyx files. 3 pyx files gave doctest failures. And I got merge conflicts with this branch.

Therefor I set this ticket to "positive review" (as suggested above).

I will open a separate ticket for == None and generate fresh patches when this ticket is merged in a beta or release.

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Apr 8, 2014

Author: Wilfried Luebbe

@vbraun
Copy link
Member

vbraun commented Apr 8, 2014

Changed branch from u/wluebbe/ticket/15984 to 6964533

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

2 participants