Skip to content
This repository has been archived by the owner on Mar 20, 2018. It is now read-only.

Add support for Python 3 #120

Merged
merged 19 commits into from
Aug 5, 2016
Merged

Add support for Python 3 #120

merged 19 commits into from
Aug 5, 2016

Conversation

geigerj
Copy link
Contributor

@geigerj geigerj commented Aug 3, 2016

Add testing environments for Python 3.4, 3.5. Modify custom iterator
objects to be Python 3-compatible.

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
Copy link
Contributor

Choose a reason for hiding this comment

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

pep8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@geigerj
Copy link
Contributor Author

geigerj commented Aug 3, 2016

Not sure why docs are not building (which is causing the CI to fail), will investigate tomorrow.

Otherwise sphinx does not import dependencies correctly.
@geigerj
Copy link
Contributor Author

geigerj commented Aug 3, 2016

Sphinx seems to want the package future to be installed in the environment that it's running in before it builds the docs for api_callable -- updated tox.ini to allow this.

@codecov-io
Copy link

codecov-io commented Aug 3, 2016

Current coverage is 97.19% (diff: 100%)

Merging #120 into master will increase coverage by 0.01%

@@             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          

Powered by Codecov. Last update 49011ba...8d243bd

@bjwatson
Copy link
Contributor

bjwatson commented Aug 3, 2016

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)
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.)

@bjwatson
Copy link
Contributor

bjwatson commented Aug 3, 2016

PLMK when the CI tests are passing for python3

@geigerj
Copy link
Contributor Author

geigerj commented Aug 3, 2016

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.
@geigerj
Copy link
Contributor Author

geigerj commented Aug 5, 2016

CI is passing!

@bjwatson
Copy link
Contributor

bjwatson commented Aug 5, 2016

Woo hoo!

@@ -251,7 +251,7 @@ def canceller():
return canceller


TIMER_FACTORY = threading.Timer
TIMER_FACTORY = threading.Timer # pylint: disable=invalid-name
Copy link
Contributor

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?

Copy link
Contributor

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',
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Fixed.

@bjwatson
Copy link
Contributor

bjwatson commented Aug 5, 2016

Just one question and then I think this is ready

@bjwatson
Copy link
Contributor

bjwatson commented Aug 5, 2016

LGTM

@geigerj geigerj merged commit d82a2bd into googleapis:master Aug 5, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants