Comment 74 for bug 893091

On Dec 13, 2011, at 11:17 AM, <email address hidden> wrote:

>> Would you rather see a patch that addresses all your comments, and which
>> obsoletes the previous patches, or would you rather see one an incremental
>> patch to apply on top of the ones you've already reviewed?
>
>Either works. To be honest, the easiest thing to review would have been a
>series of "atomic" patches (e.g. "change the base types of all int subclasses
>to be long under Python 3"), with the least controversial/most obvious changes
>first - then I could start applying the stuff I liked already, and skip the
>bits that still had problems (or depended on something problematic).

Yes, I apologize for that. I know how difficult it is to review and apply
massive diffs like the one I posted. The good news is that I see you've
already cherry picked a bunch of changes that are useful on their own, or in
preparation for Python 3 support. I've finally managed to get github working,
so I've pushed a branch containing more of these types of atomic patches:

https://github.com/warsaw/dbus-python3/tree/python3

So far, these are all changes you can accept without digesting the big Python
3 meal. They should make the eventual Python 3 patch much smaller. I'm happy
to do the difficult work of pulling things out of the big diff into smaller
diffs.

Next up: PyString -> PyBytes, which will be large-ish, but completely
compatible with Python 2. Then I'll probably do the reprs, followed by some
of the PyInt -> PyLong changes. Then we'll see where we're at. :)

>> Sounds right, I'll remove this from "user-visible changes". Since the class
>> is unused in Python 2, should I #ifdef it out when compiled under Python 2?
>
>Ideally, yes.

(re: #ifdef out _ByteBase in Python 2: will do).

>> >Putting the "none of the above" case in the middle like you have here
>...
>> >guarantees that you'll have to rewrite the conditional if you add a third
>> >thing.
>>
>> In this particular case, is there a reason not to accept either bytes or
>> unicodes in both versions of Python? My rewrite based on your feedback
>> does enable that, but if you'd rather I can add some #ifdefs to limit it to
>> "native strings" (see below).
>
>This is for signatures, right?
>
>The fact that Python 2 doesn't allow unicode signatures is arguably a bug; if
>you wanted to relax this, it's not a problem.
>
>Python 3 accepting bytes signatures would be a bit strange, but not really a
>big deal, so if you wanted to allow this, I wouldn't mind.

Cool.

>> >This ordering is really weird. Any particular reason why they're not in
>> >some sort of systematic ordering, like "sort by size and then by
>> >signedness"?
>>
>> Mostly because I copied the ordering from the Python 2 PyInt and PyLong
>> cases and then tried to combine them. I noticed the comment just before
>> this stanza:
>>
>> /* Ordering is important: some of these are subclasses of each other. */
>>
>> so I wanted to mess with that ordering as little as possible.
>
>Right, the ordering that comment refers to is (for instance) that the cases
>for Int64 and UInt64 have to come before the case for plain 'long', and the
>cases for ObjectPath and Signature have to come before the case for plain
>'str' (whatever that means in the current version).
>
>None of the user-visible dbus-python types (Int64 etc.) are subclasses of each
>other, so it's safe to re-order those.

Cool, I'll clean up the ordering when I get to that part again.

>> >This logic (to make a Byte from a single-byte unicode string) seems really
>> >strange, and a bit inefficient.
>>
>> XXX COME BACK TO ME
>
>Still to be looked at

Dang. I *knew* I'd forget to respond to that before I sent the message. I'll
get back to this one in a subsequent follow up.

>> The one reason why it silently swallows the argument is to aid in porting
>> the existing uses of utf8_strings in the Python layer of the library.
>
>Yes... but I feel as though accepting an argument that doesn't do what it's
>claimed to is rather dishonest.
>
>> It gets a little trickier at the Python layer. For example, there's a slot
>> in SignalMatch for _utf8_strings, but it's essentially ignored.
>> add_signal_receiver() also accepts but ignores it, as does call_async() and
>> call_blocking() in connection.py. There may be other cases. The question
>> is whether we want to track these down and add exceptions or deprecation
>> warnings to nudge folks into getting rid of these (and in some cases, what
>> to do about their use within the dbus-python package itself). Is it worth
>> it to complicate the Python code, or break backward compatibility?
>
>I think grepping for utf8_strings should be enough to fix the internal uses?
>Some cases will probably have to turn from
>
> foo(utf8_strings=utf8_strings)
>
>into
>
> kwargs = {}
> if is_py3 and utf8_strings:
> kwargs['utf8_strings'] = True
> foo(**kwargs)
>
>but I think that's a necessary evil.
>
>Fundamentally, anyone who's relying on utf8_strings behaviour under Python 3
>has already lost, because they can't have it.

Agreed; that's a good approach, and I'll adopt it as I port things over to the
git branch.

>> I looked into this in more detail, and AFAICT, connection.py and service.py
>> really only wants the thread module for .allocate_lock(), so this one could
>> easily be changed to use threading.Lock (or RLock?) objects.
>>
>> One thing I could not answer though is why _dbus.py imports thread. Maybe
>> we can get rid of that and switch the other two to use threading?
>
>You know Python threads better than I do, tbh...

I changed these to use threading.Lock() in the git branch.

>> >Removing the UTF8String check is a semantic change: previously we insisted
>> >that arguments matched with args_match were (in D-Bus terms) strings,
>> >whereas now you'll accept anything string-like.
>>
>> Right, so the big question for you is whether that semantic and API change
>> is acceptable.
>
>To be clear, here is the semantic change I don't like: consider this D-Bus
>match rule:
>
> "arg0='/'"
>
>and these messages:
>
> o = dbus.lowlevel.SignalMessage('/', 'a.b', 'c')
> o.append('/', signature='o') # 0'th argument is an object path
>
> s = dbus.lowlevel.SignalMessage('/', 'a.b', 'c')
> s.append('/', signature='s') # 0'th argument is a string
>
>In the D-Bus Specification, dbus-daemon, and dbus-python for Python 2, the
>match rule matches s, but not o. In dbus-python for Python 3, because you
>removed the UTF8String type-check, it will match both.
>
>Switching away from UTF8String is great, but to keep the match rule semantics
>from the D-Bus Specification, you should replace the explicit dbus.UTF8String
>check with an explicit dbus.String check, rather than removing it.

Thanks for explaining this. I'll make sure to adhere to the semantics you
describe here when I get to that part in the Python 3 port again. I'm not
sure if there are existing test cases for that; if not, I'll add one.

Again, thanks. Hopefully with the github branch we have a better workflow for
you to review and pull in the changes in more manageable chunks. If not,
please let me know! Otherwise, I'll just follow up here when I get more stuff
ready to go.