Dne 24.7.2017 v 17:32 Eduardo Habkost napsal(a): > On Mon, Jul 24, 2017 at 02:13:09PM +0200, Lukáš Doktor wrote: >> Dne 21.7.2017 v 20:42 Eduardo Habkost napsal(a): >>> On Fri, Jul 21, 2017 at 08:37:34AM +0200, Lukáš Doktor wrote: >>>> Dne 20.7.2017 v 20:27 Eduardo Habkost napsal(a): >>>>> On Thu, Jul 20, 2017 at 06:28:09PM +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> >>>>>> --- >>>>>> scripts/qemu.py | 17 +++++++++++++++-- >>>>>> 1 file changed, 15 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/scripts/qemu.py b/scripts/qemu.py >>>>>> index 5948e19..2bd522f 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__(desc) >>>>>> + self.reply = reply >>>>> >>>>> This would require every user of self.reply to first check if >>>>> it's a string or dictionary. All because of the "Monitor is >>>>> closed" case below: >>>>> >>>> I haven't used it for the `Monitor is closed` exception, so >>>> it's just to be able to store really broken reply safely. >>>> Anyway people can check whether the reply is a dict, or I can >>>> add `is_valid_reply` property which would check for >>>> `[error][desc]` presence (which is a bit more precise than just >>>> plain dict type checking). >>> >>> >>> Oops, I wasn't paying enough attention, sorry. self.reply is >>> actually always set to the response from the monitor. >>> >>> If you are just trying a safe fallback for 'desc' if the response >>> broken, what about using repr(reply) or json.dumps(reply) if >>> reply['error']['desc'] isn't set? >>> >> I could use repr, but I'd prefer keeping it the way it is as >> you could use `isinstance` to see whether it's dict and >> interact with it (if needed, eg. on negative testing, which is >> my motivation for storing the full response). > > This makes sense for self.reply, but I'm thinking about the > argument to Exception.__init__(). I'm worried that things might > break if the argument is not a string. >
I see. It's python so it'll simply use `__str__` method, but you are right that for the exception description the use of `repr` is better. I'll change it in v2 (keeping the `self.reply` as is). Lukáš
signature.asc
Description: OpenPGP digital signature
