Comment 46 for bug 846044

Revision history for this message
In , Simon McVittie (smcv) wrote :

I've applied those patches. Fixed in git for 1.1.2, unless you spot any further problems.

(In reply to comment #11)
> * I'm not sure that you need the lambda in
> test_dbus_exception_convert_str_fail().

You do. In the older form of assertRaises, the thing that raises the exception needs to be a callable, so that the exception isn't already raised before it has a chance to wrap it in a "try:".

This is much nicer in the newer form, but I don't think that's a good enough reason to require Python 2.7.

> * Can you provide some more detail about the weird up-chaining you're seeing
> with Exception.__unicode__?

It's really BaseException.__unicode__, which is this pseudocode:

    if self.__str__ isn't BaseException.__str__:
        return unicode(self.__str__()) # <---------

    if len(self.args) == 0:
        return ""

    if len(self.args) == 1:
        return unicode(self.args[0])

    # len(self.args) > 1
    return unicode(self.args)

The __str__ call will raise an exception if the message is non-ASCII but the default codec is ASCII, so we lose.

My implementation of __unicode__ is a slightly more concise version of the rest of that pseudocode - I'm essentially emulating what would happen if we didn't have a __str__ method at all. We need __str__ for Python 3, AIUI.

> * I'd probably swap this code around (I always dislike testing a negative
> when testing a positive will do just as well):
>
> + if self._dbus_error_name is not None:
> + return '%s: %s' % (self._dbus_error_name, s)
> + else:
> + return s

It was already like that, and I tend to think of "is not None" as the "positive" case - conceptually, I'm saying "if there is a _dbus_error_name". I'd accept a patch to change both copies if you feel strongly about this, though.