Add support for Python 3 #120
Conversation
Add testing environments for Python 3.4, 3.5. Modify custom iterator objects to be Python 3-compatible.
| @@ -1,8 +1,10 @@ | |||
| [tox] | |||
| envlist = py27,pep8,pylint-errors,pylint-full | |||
| envlist = py27,py34,py35,pypep8,pylint-errors,pylint-full | |||
|
Not sure why docs are not building (which is causing the CI to fail), will investigate tomorrow. |
Otherwise sphinx does not import dependencies correctly.
|
Sphinx seems to want the package |
Current coverage is 97.19% (diff: 100%)@@ master #120 diff @@
==========================================
Files 8 8
Lines 603 607 +4
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 586 590 +4
Misses 17 17
Partials 0 0
|
|
FYI @tseaver, @dhermes, @daspecster, @jgeewax, @omaray |
| """Retrieves the next resource.""" | ||
| # pylint: disable=next-method-called | ||
| while not self._current: | ||
| self._current = self._iterator.next() | ||
| self._current = next(self._iterator) |
There was a problem hiding this comment.
Why is this change being made? It's a bit confusing, since it looks like it's calling ResourceIterator.next(), but I think it's really calling PageIterator.next().
It would help to rename self._iterator to self._page_iterator.
There was a problem hiding this comment.
Python 3 style is for iterators to have a special method __next__ and then invoke the built-in function next() to call it.
The next() method is retained as an alias only for Python 2 compatibility -- in Python 3, the iterator's next method is not meant to be invoked directly.
Renamed per your suggestion.
There was a problem hiding this comment.
I see. Is this a canonical pattern for Python2/3 compatible code, or should it be explained more in the iterator next() docstrings?
There was a problem hiding this comment.
I would expect that it's the common way of doing, since there's really only one right way of implementing a custom iterator in each of Python 2 and 3, but I don't really know enough to be able to call it "canonical".
FWIW, this is also basically the way that is recommended by the writers of the future library, which is a well-known library for 2/3 intercompatibility: http://python-future.org/compatible_idioms.html#custom-iterators. (They don't show the aliasing of the old next() method, but the presence of that method actually is necessary for Python 2 to recognize that this object is an iterator.)
Per PR feedback
|
PLMK when the CI tests are passing for python3 |
|
Still trying to figure out why the CI is failing -- will update when it's working |
It would be nice if this worked, but it should be OK as long as we lint once for each commit and run the tests in all Python versions.
This is just a temporary commit to help debug CI failures
Still trying to debug CI failure...
This pulls in protoc, as before, but also grpc_python_plugin, which the grpcio installation seems to require for python 3.
|
CI is passing! |
|
Woo hoo! |
| @@ -251,7 +251,7 @@ def canceller(): | |||
| return canceller | |||
|
|
|||
|
|
|||
| TIMER_FACTORY = threading.Timer | |||
| TIMER_FACTORY = threading.Timer # pylint: disable=invalid-name | |||
There was a problem hiding this comment.
does this mean something, or was it a debugging comment?
There was a problem hiding this comment.
NVM, I just realized that this is an instruction to pylint
| @@ -49,7 +49,7 @@ | |||
| raise RuntimeError("No version number found!") | |||
|
|
|||
| install_requires = [ | |||
| 'grpcio>=0.15.0', | |||
| 'grpcio>=1.0rc1', | |||
There was a problem hiding this comment.
Is future needed here, too?
|
Just one question and then I think this is ready |
|
LGTM |
Add testing environments for Python 3.4, 3.5. Modify custom iterator
objects to be Python 3-compatible.