Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python 3 Compatibility #108

Closed
wants to merge 6 commits into from
Closed

Python 3 Compatibility #108

wants to merge 6 commits into from

Conversation

surfacepatterns
Copy link

Even though I'm making a pull request, it's probably not reasonable to accept the pull request yet. I say this because:

  1. The thrift compiler generated incorrect Python 3 bindings up until eight weeks ago, when this commit was made. The commit made it so that absolute imports weren't used to import submodules in generated bindings.
  2. Bindings generated by code in the thrift master branch reference classes that are not necessarily available in earlier versions of python thrift bindings.

In consequence, I'd probably wait for a new release of Thrift before accepting the pull request.

Aside from that, I should say a few things about my changes:

  1. The Hbase.thrift file indicates the bindings should accept unicode arguments in types marked as 'Text', but generated python bindings accept bytes objects. I decided that the happybase API should still do the right thing in those cases; hence, str.encode() and bytes.decode() are used pretty liberally.
  2. Some functions that normally return an iterator in Python 2 aren't available in Python 3 (e.g. xrange(), dict.iteritems(), etc.). Sometimes, I made the decision to create a convenient alias for Python 3's sake; other times, I chose to use a less efficient method in Python 2. Feel free to make different choices.
  3. I updated the Hbase.thrift file to the newest one in the hbase repository. I then updated the bindings using the thrift compiler from the current master branch.
  4. The happybase test suite was run using both Python 2 and 3. All tests in the test suite passed in both cases. I have not tested the bindings under any other scenario (yet).

Let me know if you have any questions.

Devin Anderson added 6 commits March 8, 2016 10:44
…ate Hbase bindings using new definitions file.
…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.
…lueError instead of TypeError when passing None where an integral type is expected.
@surfacepatterns surfacepatterns mentioned this pull request Mar 9, 2016
3 tasks
@surfacepatterns
Copy link
Author

I've done some reading, and have found that most of the Hbase documentation conflicts with the comment in the Hbase.thrift file that suggests that keys, values, etc. should be UTF-8. I'm closing this pull request, and will make another pull request which accepts bytes() instances in Python 3.

@scttcper
Copy link

@surfacepatterns keep it up. Would really like to see python3 support even if it doesn't make it into master.

@surfacepatterns
Copy link
Author

Created a new pull request that does the right thing.

@wbolster
Copy link
Member

yeah data is always bytes and happybase should NOT accept anything else.

helper methods like table names or keys in config dicts are a different story, accepting "native" strings makes sense wherever reasonable.

— wouter

sent from my phone. please forgive my tpyos.

On March 10, 2016 10:16:12 PM GMT+01:00, Devin Anderson notifications@github.com wrote:

I've done some reading, and have found that most of the Hbase
documentation conflicts with the comment in the Hbase.thrift file that
suggests that keys, values, etc. should be UTF-8. I'm closing this
pull request, and will make another pull request which accepts bytes()
instances in Python 3.


Reply to this email directly or view it on GitHub:
#108 (comment)

@wbolster
Copy link
Member

it will make it into master. i have a dislike for py2 nowadays 😉

— wouter

sent from my phone. please forgive my tpyos.

On March 10, 2016 10:27:21 PM GMT+01:00, Scott Cooper notifications@github.com wrote:

@surfacepatterns keep it up. Would really like to see python3 support
even if it doesn't make it into master.


Reply to this email directly or view it on GitHub:
#108 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants