[BEAM-3999] Futurize internal subpackage #5334
Conversation
|
retest this please |
| import logging | ||
| import threading | ||
| import weakref | ||
| from builtins import next |
There was a problem hiding this comment.
I'd like to understand the reason for adding from builtins import next. Is it done by a conversion tool because we have an occurrence of next(v_iter) in the file, and the tool cannot infer whether or not v_iter implements a custom iterator? If yes: will such changes be required by linter?
Is my understanding correct that as long as v_iter.__class__ does not implement a custom iterator (and it is not, in our case), the code without the import is still equivalent in Python 2 and Python 3?
There was a problem hiding this comment.
This is indeed done by the automatic conversion of future.
And you're right, it isn't necessary here. Lint doesn't seem to complain either, so we can remove this.
There was a problem hiding this comment.
Hm... the change is harmless, I totally don't mind it. I am a little concerned that we might not be able to enforce consistency in the codebase, and automatically catch when such changes are required. Say, tomorrow somebody adds a usage of next in against an object passed into the method, and we can't tell if the object will has the iterator implemented in Py3-compliant way, so then there would be a difference in behavior between py2 and py3, and importing next would make a difference, but if linter does not catch it, chances are reviewers won't notice it either.
There was a problem hiding this comment.
I checked again, and it seems like the linter checks for .next() calls on objects. So we should be safe.
|
retest this please |
|
Let's see if the Py3 linter kicks in. |
|
The linter did run before pushing my new changes. |
aaltay
left a comment
aaltay
left a comment
There was a problem hiding this comment.
Generally LGTM, could you rebase please?
sdks/python/tox.ini
Outdated
| [tox] | ||
| # new environments will be excluded by default unless explicitly added to envlist. | ||
| envlist = py27,py27-{gcp,cython,lint,lint3},py3-lint,docs | ||
| #envlist = py27,py27-{gcp,cython,lint,lint3},py3-lint,docs |
3 participants
This pull request prepares the internal subpackage for Python 3 support. This PR is part of a series in which all subpackages will be updated using the same approach.
This approach has been documented here and the first pull request in the series (Futurize coders subpackage) demonstrating this approach can be found at #5053.
R: @aaltay @tvalentyn