Re: [Qemu-devel] [PATCH v2 05/10] qemu.py: Use custom exceptions rather than Exception

[Eduardo Habkost]

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 <address@hidden>
> >> Reviewed-by: Eduardo Habkost <address@hidden>
> >> ---
> >>  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