More backwards-compatible Python 3 changes

kozec / syncthing-gtk

GTK3 & python based GUI for Syncthing

GNU General Public License v2.0

1.29k stars 140 forks source link

More backwards-compatible Python 3 changes #469

Closed dralley closed 6 years ago

dralley commented 6 years ago

These are very slightly more invasive than the previous set (but not enormously so), and as such deserve a bit more scrutiny.

But FWIW, I did a little testing on my machine and everything worked fine.

Made all relative imports explicit
In places where dict.keys() and dict.values() are used with list operations, wrap them with list() first
- In Python 3, dict.keys() and dict.values() behave just like xrange(), or dict.iterkeys()/dict.itervalues(). Therefore, while iterating over them is exactly the same, if you want to do list operations with them, you have to convert them into lists first.
Changed xrange() to range()
- For small range sizes, the difference should be imperceptible

More detail in commit messages / comments.

kozec commented 6 years ago

As I explained in that original PR, turning xrange to range is not backwards compatible, as it does different thing in python2. Even if we assume that it's "just a little bit of memory", there will be eventually case where it will do just a huge chunk of it, so this is not a solution :(

dralley commented 6 years ago

@kozec re: xrange(), well, I could drop the commit entirely and leave it for dealing with later, or we could do a from builtins import range so that range() would behave just like xrange().

kozec commented 6 years ago

we could do a from builtins import range so that range() would behave just like xrange().

What kind of wizardry is this? :)

But it sounds like a good solution.

// edit: If it doesn't break Py3, from future.builtins import range would be more explicit.

dralley commented 6 years ago

Argh, apparently builtins isn't always included with the system Python unless you install the python2-future package (even though from __future__ works fine). At least not on Fedora. I think maybe it's installed by default elsewhere.

I'll just drop it from this PR and save it for a future one. I'm sure it can be just added to the requirements easily (future exists on PyPI also), but I'm trying to keep the individual PRs somewhat minimal change-wise.

dralley commented 6 years ago

Commit dropped

dralley commented 6 years ago

For what it's worth, even range(1000000) (1 million) only uses about 8mb of memory, which is garbage collected soon after it's finished being iterated over.

I don't think it'd ever be a problem, but I defer to your judgement. There's definitely ways to make it work your way.

kozec commented 6 years ago

Merged (along with # 470), thanks :)