On Wed, Jul 26, 2017 at 06:39:28AM +0200, Lukáš Doktor wrote:
> Dne 25.7.2017 v 18:06 Eduardo Habkost napsal(a):
> > On Tue, Jul 25, 2017 at 05:09:46PM +0200, Lukáš Doktor wrote:
> >> The naked Exception should not be widely used. It makes sense to be a
> >> bit more specific and use better-suited custom exceptions. As a benefit
> >> we can store the full reply in the exception in case someone needs it
> >> when catching the exception.
> >>
> >> Signed-off-by: Lukáš Doktor <ldok...@redhat.com>
> >> Reviewed-by: Eduardo Habkost <ehabk...@redhat.com>
> >> ---
> >> scripts/qemu.py | 17 +++++++++++++++--
> >> 1 file changed, 15 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/scripts/qemu.py b/scripts/qemu.py
> >> index 5948e19..e6df54c 100644
> >> --- a/scripts/qemu.py
> >> +++ b/scripts/qemu.py
> >> @@ -19,6 +19,19 @@ import subprocess
> >> import qmp.qmp
> >>
> >>
> >> +class MonitorResponseError(qmp.qmp.QMPError):
> >> + '''
> >> + Represents erroneous QMP monitor reply
> >> + '''
> >> + def __init__(self, reply):
> >> + try:
> >> + desc = reply["error"]["desc"]
> >> + except KeyError:
> >> + desc = reply
> > ^^^ (*)
> >> + super(MonitorResponseError, self).__init__(repr(desc))
> >
> > This will end up calling Exception.__init__. I previously
> > suggested repr(desc) above(*) because I didn't know what happened
> > when the Exception.__init__ argument is not a string.
> >
> > I couldn't find any documentation on the right argument types for
> > Exception.__init__. The examples in the Python documentation[1]
> > don't call Exception.__init__ at all: they simply implement
> > __str__().
> >
> > However, based on my testing and on the BaseException
> > documentation[2], I believe repr() isn't really necessary:
> >
> > "If str() or unicode() is called on an instance of this class,
> > the representation of the argument(s) to the instance are
> > returned, or the empty string when there were no arguments."
> >
> > So your previous version was good, already.
> >
> > But maybe we could do this instead, to make the constructor as
> > simple as possible:
> >
> > class MonitorResponseError(qmp.qmp.QMPError):
> > def __init__(self, reply):
> > self.reply = reply
> >
> > def __str__(self):
> > return self.reply.get('error', {}).get('desc', repr(self.reply))
> >
> Well I know I can implement custom `__str__` class, but the
> default one simply stores the argument as string and reports
> it, so for simple strings I usually go with super call and with
> complex ones with custom `__str__`. Anyway if this suits this
> project style better I'll change it in v2.
If you prefer it, your previous version (without repr) was good
enough to me.
--
Eduardo
