Skip to content

Python 3: zip function -> list(zip), itertools.izip -> zip() - #9729

Merged
michaelDCurran merged 5 commits into
nvaccess:threshold_py3_stagingfrom
josephsl:py3izip2zip
Jun 14, 2019
Merged

Python 3: zip function -> list(zip), itertools.izip -> zip()#9729
michaelDCurran merged 5 commits into
nvaccess:threshold_py3_stagingfrom
josephsl:py3izip2zip

Conversation

@josephsl

Copy link
Copy Markdown
Contributor

Hi,

Discovered through a series of failing unit tests:

Link to issue number:

None

Summary of the issue:

Python 2 and 3 zip functions have different behaviors: returns a list in Python 2 vs an iterator in Python 3.

Description of how this pull request fixes the issue:

Changed instances of zip() function call to list(zip), as well as rename itertools.izip to zip().

Steps:

  1. Grep "zip(" source.
  2. First, wrap language handler zip function inside a list call.
  3. Convert itertools.izip to zip and remove itertools import from IA1 mozilla text module.

Testing performed:

Tested in Python 2 and 3 interpreters.

Known issues with pull request:

None

Change log entry:

None

josephsl added 3 commits June 12, 2019 14:57
Behavior of zip() functoin has changed - returning a list in Python 2 versus being an iterator in Python 3. Because language handler/language list uses old zip function behavior, wrap this inside a list call.
@josephsl
josephsl requested a review from michaelDCurran June 12, 2019 22:15
# Find the first common ancestor.
maxAncIndex = min(len(selfAncs), len(otherAncs)) - 1
for (selfAncTi, selfAncObj), (otherAncTi, otherAncObj) in itertools.izip(selfAncs[maxAncIndex::-1], otherAncs[maxAncIndex::-1]):
for (selfAncTi, selfAncObj), (otherAncTi, otherAncObj) in zip(selfAncs[maxAncIndex::-1], otherAncs[maxAncIndex::-1]):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please split this line while at it

Comment thread source/languageHandler.py Outdated
#Prepare a zipped view of language codes and descriptions.
# #7284: especially for sorting by description.
langs = zip(locales,displayNames)
# #7105 (Py3 review required): Python 2 list 0> Python 3 iterator.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this comment is necessary. Furthermore, it contains a typo.

…ndler, pslit lines in mozilla text iA2 objects.
@josephsl

josephsl commented Jun 13, 2019 via email

Copy link
Copy Markdown
Contributor Author

@michaelDCurran

Copy link
Copy Markdown
Member

@LeonarddeR are you still waiting for changes on this, or are you going to approve?

@LeonarddeR LeonarddeR left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, go ahead.

@michaelDCurran
michaelDCurran merged commit 8d63788 into nvaccess:threshold_py3_staging Jun 14, 2019
@nvaccessAuto nvaccessAuto added this to the 2019.3 milestone Jun 14, 2019
@michaelDCurran

Copy link
Copy Markdown
Member

After merging this, setup.py failed as it found a syntax error in source\NVDAObjects\IAccessible\ia2TextMozilla.py, introduced by this pr. Due to a for loop being split across two lines.
I have manually fixed this in 5da17fd, which is now on threshold_py3_staging.
I appreciate that shorter line lengths are easier to read for some people, but I would suggest we back down on this a bit, especially with reviewing code for the Python3 transition. Not only was there this syntax error, but a lot of other code is having to be slightly rewritten, which really has nothing to do with Python3 and could introduce further bugs.

@LeonarddeR

LeonarddeR commented Jun 14, 2019 via email

Copy link
Copy Markdown
Collaborator

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants