Python 3 Compatibility (Part 2) #111

Closed
wants to merge 10 commits into
from

Projects

None yet

2 participants

@surfacepatterns

These commits introduce Python 3 compatibility into happybase.

surfacepatterns added some commits Mar 8, 2016
@surfacepatterns surfacepatterns Update Hbase.thrift definitions to latest upstream definitions. Gener…
…ate Hbase bindings using new definitions file.
a1a9ac1
@surfacepatterns surfacepatterns Add Python 3 support to happybase. Mostly, this meant tedious encodin…
…g and decoding of strings and bytes respectively, but also meant correcting imports and, in a couple cases, explicitly doing something different for Python 2 vs. Python 3.
cc6790b
@surfacepatterns surfacepatterns Make test suite Python 3 compatible. Fix one test that checked for Va…
…lueError instead of TypeError when passing None where an integral type is expected.
bdb0a05
@surfacepatterns surfacepatterns Remove execfile() - use imp.load_source() to get version info instead. bbcca36
@surfacepatterns surfacepatterns Add Makefile targets for Python 3. c1f65b5
@surfacepatterns surfacepatterns Remove TODO item for Python 3 compatibility. 85a18ba
@surfacepatterns surfacepatterns Revert Python 3 support so that it can be re-done such that the str.e…
…ncode() and bytes.decode() operations don't take place, as, despite what the Hbase.thrift file might tell you, Hbase expects binary data, not UTF-8.

This reverts commit cc6790b.
68c03e6
@surfacepatterns surfacepatterns Revert "Make test suite Python 3 compatible. Fix one test that checke…
…d for ValueError instead of TypeError when passing None where an integral type is expected."

This reverts commit bdb0a05.
d03727e
@surfacepatterns surfacepatterns Make happybase module Python 3 compatible. This basically entailed:
  * Replacing dict.iteritems() calls with dict.items().
  * Making functions check for `bytes` instead of `str`.
  * Replacing `str` literals with `bytes` literals where appropriate.
  * Changing imports when Python 2 modules aren't available in Python 3.
  * Surrounding map() calls with list() where sequences are expected.
8ea4984
@surfacepatterns surfacepatterns Make test suite run with Python 2 and Python 3. This basically entails:
  * Creating an `xrange` alias for Python 3.
  * Replacing `str` literals with `bytes` literals where appropriate.
  * Changing one test to expect `TypeError` instead of `ValueError`.
  * Using the `codecs` module for decoding/encoding hex.
b461bb1
This was referenced Mar 11, 2016
@wbolster wbolster commented on the diff Mar 19, 2016
doc/conf.py
@@ -48,11 +49,13 @@
# built documents.
#
# The short X.Y version.
-execfile(os.path.join(os.path.dirname(__file__), '../happybase/_version.py'))
-version = __version__
+
+version_path = join(dirname(__file__), '../happybase/_version.py')
+with open(version_path) as fp:
+ version = imp.load_source("_version", version_path, fp).__version__
@wbolster
wbolster Mar 19, 2016 Owner

imp.load_source() is not public api in python 3, and the imp module is deprecated and pending removal...

@wbolster
wbolster Mar 19, 2016 Owner

fwiw, the setup.py has this:

__version__ = None
exec(open('happybase/_version.py', 'r').read())
@surfacepatterns
surfacepatterns Mar 20, 2016

exec() seems like overkill. If I had my way, I would likely put the version (and any other metadata about the module) into a separate file (json, ini, etc.), and read the data from there.

@wbolster wbolster commented on the diff Mar 19, 2016
@@ -8,12 +8,25 @@ doc:
@echo Generated documentation: "file://"$$(readlink -f doc/build/html/index.html)
@echo
+doc3:
+ python3 setup.py build_sphinx
+ @echo
+ @echo Generated documentation: "file://"$$(readlink -f doc/build/html/index.html)
+ @echo
+
@wbolster
wbolster Mar 19, 2016 Owner

this part is not really needed. a separate tox environment for building the docs is perhaps a better solution.

@surfacepatterns
surfacepatterns Mar 20, 2016

I don't know what tox is. Do whatever you think is reasonable for your development needs.

@wbolster wbolster commented on the diff Mar 19, 2016
clean:
find . -name '*.py[co]' -delete
dist: test
python setup.py sdist
+
+dist3: test3
+ python3 setup.py sdist
@wbolster
wbolster Mar 19, 2016 Owner

this target is not needed at all.

@surfacepatterns
surfacepatterns Mar 20, 2016

You're right. I've been using python3 in my invocations for building the package using stdeb, and blindly copied it in for creating the source distribution.

@wbolster wbolster commented on the diff Mar 19, 2016
test:
-find coverage/ -mindepth 1 -delete
python $$(which nosetests) $${TESTS}
+test3:
+ -find coverage/ -mindepth 1 -delete
+ python3 $$(which nosetests3) $${TESTS}
+
@wbolster
wbolster Mar 19, 2016 Owner

this target should go. a separate tox environment is the preferred way to test against multiple python versions.

these makefile targets only exist because it makes development from within a virtualenv easier (no need to run tox which is slower). and within a virtualenv, python points to the correct one: there's no python vs python3.

@surfacepatterns
surfacepatterns Mar 20, 2016

Again, I have no knowledge of tox. I did, however, use the test3 target extensively to progressively test my changes in Python 3.

@wbolster wbolster commented on the diff Mar 19, 2016
happybase/Hbase.thrift
@@ -75,7 +75,7 @@ struct ColumnDescriptor {
6:i32 bloomFilterVectorSize = 0,
7:i32 bloomFilterNbHashes = 0,
8:bool blockCacheEnabled = 0,
- 9:i32 timeToLive = -1
+ 9:i32 timeToLive = 0x7fffffff
@wbolster
wbolster Mar 19, 2016 Owner

am i right in assuming that this is a unmodified upstream version of the thrift file?

@wbolster wbolster commented on the diff Mar 19, 2016
happybase/batch.py
@@ -45,7 +45,7 @@ def _reset_mutations(self):
def send(self):
"""Send the batch to the server."""
- bms = [BatchMutation(row, m) for row, m in self._mutations.iteritems()]
+ bms = [BatchMutation(row, m) for row, m in self._mutations.items()]
@wbolster
wbolster Mar 19, 2016 Owner

this makes copies into a new list which iterated over and then thrown away in python 2 :(

@surfacepatterns
surfacepatterns Mar 20, 2016

Yes. I thought about creating a separate utility function just for iterating over items:

if sys.version_info >= (3,):
    def iteritems(d):
        return d.items()
else:
    def iteritems(d):
        return d.iteritems()

PEP 469 recommends this approach, though also mentions that six and future.utils provide the same functionality.

@wbolster wbolster commented on the diff Mar 19, 2016
tests/test_api.py
@@ -43,13 +44,15 @@
# Yuck, globals
connection = table = None
+if sys.version_info >= (3,):
+ xrange = range
@wbolster
wbolster Mar 19, 2016 Owner

i would prefer the reverse logic. python 3 is now. python 2 is the past. :)

@surfacepatterns
surfacepatterns Mar 20, 2016

I feel the same way, though I'm generally cautious about overwriting a module's reference to a built-in in another developer's code.

@wbolster
Owner

note for readers: #108 contains an earlier stab at this, and also has some interesting comments and observations.

@wbolster
Owner

also, the python thrift project is a bit of a pain.

switching to thriftpy may be a wise choice. https://thriftpy.readthedocs.org/en/latest/

@wbolster
Owner

porting to thriftpy was easier than it seemed: see #114 . feedback more than welcome!

@surfacepatterns

The thrift python bindings are disgustingly un-pythonic.

I'll take a look at #114 later. It's nearly 5:00 AM my time. I should be sleeping. :)

@wbolster
Owner

status update: i have some local unfinished work wrt thriftpy and python 3 support, heavily based on this pr but not exactly the same.

i think it makes sense for me to continue that work instead of polishing this branch/pr since i already cherry-picked all the good work done by you. :)

are there any issues or further thoughts in addition to what's already covered in this pr (both code and discussions), and the info captured in the discussion about the previous attempt in #108 ?

@wbolster
Owner

in case you're interested, i've published a dev and a python3 branch which are both unfinished wip branches which i'll rebase to pieces at some point anyway.

to be continued.

@wbolster
Owner
wbolster commented Aug 1, 2016

thanks @surfacepatterns for your hard work on this. your contributions helped a lot to get python 3 support in. 👍

i'm closing this since i merged #116 which closes #40.

@wbolster wbolster closed this Aug 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment