Merge lp://staging/~zyga/launchpadlib/fix-1471927 into lp://staging/launchpadlib

You say that this is ensuring that it's not unicode on Python 2, but if you pass unicode in then it will try to decode it and (depending on the contents) either crash or leave it as unicode. Either way it's a bit inconsistent with the comment.

I haven't looked at the context further down, but can you just decode if it's bytes and drop the not str condition? You'll get unicode on Python 2, but it should tolerate that fine here; and it would be simpler code requiring a simpler comment (or probably none at all).

Er, wait, it won't try to decode it because of the first test, so that part of my comment is wrong, sorry. I'd still rather see the simpler version if it works - could you check that?

Yep, certainly. I think nothing ever calls the unicode code path there
so in reality you either get str -> str (on python2) or you get bytes
-> str (on python3). The complex test was so that I don't introduce
str -> unicode (on python2) that people may not expect to handle.

On Tue, Jul 7, 2015 at 2:02 AM, Colin Watson <email address hidden> wrote:
> Er, wait, it won't try to decode it because of the first test, so that part of my comment is wrong, sorry. I'd still rather see the simpler version if it works - could you check that?
> --
> https://code.launchpad.net/~zyga/launchpadlib/fix-1471927/+merge/263953
> You are the owner of lp:~zyga/launchpadlib/fix-1471927.

In a few other places in that file, the following is done:

        if not isinstance(value, unicode_type):
            value = value.decode('utf-8')

the assumption being that although over the wire things are bytes, we expect them to actually be unicode in most places and try to decode to native string straight away.

Wouldn't above pattern work here as well?

Will check later when reviewing other stuff.

Right the query_string should be string here. Check for "bytes & ! string" is redundant, cause in python2 both are the same. Simple check for ! string should be sufficient then. Pushed new branch with a fix, i will request a review from zyga.

This was superseded by https://code.launchpad.net/~xnox/launchpadlib/fix-1471927/+merge/264480, merged some years ago. I'll reject this one.