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 <ldoktor@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@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))
+ self.reply = reply
+
+
class QEMUMachine(object):
'''A QEMU VM'''
@@ -197,9 +210,9 @@ class QEMUMachine(object):
'''
reply = self.qmp(cmd, conv_keys, **args)
if reply is None:
- raise Exception("Monitor is closed")
+ raise qmp.qmp.QMPError("Monitor is closed")
if "error" in reply:
- raise Exception(reply["error"]["desc"])
+ raise MonitorResponseError(reply)
return reply["return"]
def get_qmp_event(self, wait=False):
--
2.9.4
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 <ldoktor@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@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))
[1] https://docs.python.org/2/tutorial/errors.html#user-defined-exceptions
[2] https://docs.python.org/2/library/exceptions.html#exceptions.BaseException
> + self.reply = reply
> +
> +
> class QEMUMachine(object):
> '''A QEMU VM'''
>
> @@ -197,9 +210,9 @@ class QEMUMachine(object):
> '''
> reply = self.qmp(cmd, conv_keys, **args)
> if reply is None:
> - raise Exception("Monitor is closed")
> + raise qmp.qmp.QMPError("Monitor is closed")
> if "error" in reply:
> - raise Exception(reply["error"]["desc"])
> + raise MonitorResponseError(reply)
> return reply["return"]
>
> def get_qmp_event(self, wait=False):
> --
> 2.9.4
>
--
Eduardo
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 <ldoktor@redhat.com>
>> Reviewed-by: Eduardo Habkost <ehabkost@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.
Lukáš
>
> [1] https://docs.python.org/2/tutorial/errors.html#user-defined-exceptions
> [2] https://docs.python.org/2/library/exceptions.html#exceptions.BaseException
>
>> + self.reply = reply
>> +
>> +
>> class QEMUMachine(object):
>> '''A QEMU VM'''
>>
>> @@ -197,9 +210,9 @@ class QEMUMachine(object):
>> '''
>> reply = self.qmp(cmd, conv_keys, **args)
>> if reply is None:
>> - raise Exception("Monitor is closed")
>> + raise qmp.qmp.QMPError("Monitor is closed")
>> if "error" in reply:
>> - raise Exception(reply["error"]["desc"])
>> + raise MonitorResponseError(reply)
>> return reply["return"]
>>
>> def get_qmp_event(self, wait=False):
>> --
>> 2.9.4
>>
>
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 <ldoktor@redhat.com>
> >> Reviewed-by: Eduardo Habkost <ehabkost@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
© 2016 - 2026 Red Hat, Inc.