[GitHub] thrift pull request #1289: fix TypeError when a py3 str is passed to the fun...

[chaoyann <gi...@git.apache.org>]

GitHub user chaoyann opened a pull request:

    https://github.com/apache/thrift/pull/1289

    fix TypeError when a py3 str is passed to the function.

    str of py3 is not instance of bytes, a Error will rise because cannot write a object to buffer

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/chaoyann/thrift THRIFT-1857

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/thrift/pull/1289.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #1289
    
----
commit 2bcac2bf027257206f52f9b985fc7ea0a022448b
Author: chaoyann <jo...@qq.com>
Date:   2017-06-14T10:53:10Z

    fix TypeError when a py3 str is passed to the function.
    str of py3 is not instance of bytes

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] thrift issue #1289: fix TypeError when a py3 str is passed to the function.

[jeking3 <gi...@git.apache.org>]

Github user jeking3 commented on the issue:

    https://github.com/apache/thrift/pull/1289
  
    Based on what I've read about py3 strings, and the fact that thrift strings are designated to be encoded as UTF8, this change seems correct.  I'm not so sure about what happens on the other side (decoding) however my guess is ti is okay.
    
    @nsuke since you did a bunch of work on the python TBinaryProtocol could you look at this?


---

[GitHub] thrift issue #1289: fix TypeError when a py3 str is passed to the function.

[nsuke <gi...@git.apache.org>]

Github user nsuke commented on the issue:

    https://github.com/apache/thrift/pull/1289
  
    Basically yes with some note.
    
    It's typically not a UTF-8 encoded binary. To write utf8 strings we can use `writeString`. The point is that `writeBinary` is for writing binaries.
    
    The real problem here is that py2 `str` *is* `byte` but py3 `str` is not. This forces many python users to put this kind of conversions everywhere when migration from 2 to 3. Putting this in Apache Thrift will save their time, while introducing potential surprises to another set of users.



---

[GitHub] thrift issue #1289: fix TypeError when a py3 str is passed to the function.

[jeking3 <gi...@git.apache.org>]

Github user jeking3 commented on the issue:

    https://github.com/apache/thrift/pull/1289
  
    Could you open a Jira ticker for this?
    
    https://thrift.apache.org/docs/HowToContribute
    
    Also could you rebase on master and refresh the PR so we get a clean build, now that CI is stable again? Thanks.


---

[GitHub] thrift issue #1289: fix TypeError when a py3 str is passed to the function.

[jeking3 <gi...@git.apache.org>]

Github user jeking3 commented on the issue:

    https://github.com/apache/thrift/pull/1289
  
    @nsuke there must be some common wisdom and/or tooling to facilitate the conversion from py2 to py3 in this regard - what have other projects done when facing this issue?


---

[GitHub] thrift issue #1289: fix TypeError when a py3 str is passed to the function.

[nsuke <gi...@git.apache.org>]

Github user nsuke commented on the issue:

    https://github.com/apache/thrift/pull/1289
  
    It's more of a usability problem.
    
    The problem is that the user is passing a non-`byte` instance to `writeBinary`. It's basically the same error as when you pass str `'42'` (not integer `42`) to writeI32.
    
    We can put this kind of conversion to any `writeXxx` functions including `writeBinary`. A downside would be potential surprises when users didn't actually intend to do that.


---

[GitHub] thrift issue #1289: fix TypeError when a py3 str is passed to the function.

[jeking3 <gi...@git.apache.org>]

Github user jeking3 commented on the issue:

    https://github.com/apache/thrift/pull/1289
  
    @nsuke it sounds like you would recommend that we do not accept this fix, and have people do their own conversion before trying to add a string, to make sure it is a byte string (UTF-8 encoded).  Correct?


---

[GitHub] thrift issue #1289: fix TypeError when a py3 str is passed to the function.

[jeking3 <gi...@git.apache.org>]

Github user jeking3 commented on the issue:

    https://github.com/apache/thrift/pull/1289
  
    Could you open a Jira ticker for this?
    
    https://thrift.apache.org/docs/HowToContribute
    
    Also could you rebase on master and refresh the PR so we get a clean build, now that CI is stable again?  Thanks.


---