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

Replaced .iteritems() with six.iteritems() for Python 2 and 3 compat #274

Merged
merged 1 commit into from
Jul 24, 2019
Merged

Conversation

igormp
Copy link
Contributor

@igormp igormp commented Jul 17, 2019

Fixed some python 2 leftovers, as discussed in #267.
Also formatted code according to Black, I hope that's not a problem.

This possibly breaks some python 2 compatibility.

@igormp
Copy link
Contributor Author

igormp commented Jul 17, 2019

As an addendum, I've noticed the usage of six.iteritems() in order to keep both python 2 and 3 compatibility. I wasn't sure if I should've kept using it instead of .items(), but I can't see why we should support Python 2 anyway.

@tataganesh
Copy link
Member

@igormp Thank you for the contribution. But isn't .items() supported in both Python2 and Python 3?

@igormp
Copy link
Contributor Author

igormp commented Jul 21, 2019

It is indeed, but might be way slower in Python 2 (at least according to this).

@pietermarsman
Copy link
Member

but I can't see why we should support Python 2 anyway.

For now we support both python 2 and 3. Perhaps that will be changed in the future. Dropping python 2 is discussed (as you already know) in #194.

Also formatted code according to Black, I hope that's not a problem.

Only format the lines you actually change. Formatting lines written by others messes up the git line history which can come in handy if you want to figure out why things are made in a particular way.

As an addendum, I've noticed the usage of six.iteritems() in order to keep both python 2 and 3 compatibility.

As I see it there are 3 options: .iteritems(), .items(), and six.iteritems(). The does not exist in python 3, so we should not use that. There are still 8 occurances of .iteritems() and they should be replaced in this PR. .items() only works well in python 3, in python 2 it can cause memory issues because it returns a list instead of a generator, so we should not use it. I think six.iteritems() is the best solution because it works in both 2 and 3 and does not have memory issues.

In short: I suggest we replace all occurances of .iteritems() and .items() with six.iteritems().

@pietermarsman
Copy link
Member

This is an (active) duplicate of #123.

pdfminer/cmapdb.py Outdated Show resolved Hide resolved
pdfminer/pdfdevice.py Outdated Show resolved Hide resolved
@igormp
Copy link
Contributor Author

igormp commented Jul 24, 2019

Ok, redid it. I can squash the commits after someone reviews the new changes.

I also think that it's a good idea to rename the PR since we're going to keep six.

Copy link
Member

@pietermarsman pietermarsman left a comment

Choose a reason for hiding this comment

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

This PR is looking good, it is a clear improvement and makes our code more consistent. I do not have any comments.

This is a squashed commit, the previous messages can be seen bellow

This is the 1st commit message:

Replaced .iteritems() usage for .items()

Fixed some python 2 leftovers, as discussed in #267. Also formatted code according to Black.\nThis possibly breaks some python 2 compatibility

This is the commit message #2:

Reverted formatting and more spread six usage
@igormp igormp changed the title Replaced .iteritems() usage for .items() Replaced .iteritems() and with six.iteritems() for Python 3 compat Jul 24, 2019
@igormp
Copy link
Contributor Author

igormp commented Jul 24, 2019

Squashed the commits and changed the title, everything is good to go.

@igormp igormp changed the title Replaced .iteritems() and with six.iteritems() for Python 3 compat Replaced .iteritems() with six.iteritems() for Python 2 and 3 compat Jul 24, 2019
@tataganesh tataganesh merged commit 17364aa into pdfminer:develop Jul 24, 2019
@igormp igormp deleted the iteritems branch July 24, 2019 17:53
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.

3 participants