-
Notifications
You must be signed in to change notification settings - Fork 13
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pep8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this mean something, or was it a debugging comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is future
needed here, too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Fixed.
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.