Re: [PATCH v2 1/8] qapi/error: Repurpose QAPIError as a generic exception base class

[John Snow]

On 4/16/21 2:04 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
> > On 4/15/21 2:44 AM, Markus Armbruster wrote:
> > > John Snow <jsnow@redhat.com> writes:
> > >
> > > > Rename QAPIError to QAPISourceError, and then create a new QAPIError
> > > > class that serves as the basis for all of our other custom exceptions.
> > >
> > > Isn't the existing QAPIError such a base class already?  Peeking
> > > ahead...  aha, your new base class is abstract.  Can you explain why we
> > > that's useful?
> >
> > Sure. The existing QAPIError class assumes that it's an error that has a
> > source location, but not all errors will. The idea is that an abstract
> > error class can be used as the ancestor for all other errors in a
> > type-safe way, such that:
> >
> > try:
> >       qapi.something_or_other()
> > except QAPIError as err:
> >       err.some_sort_of_method()
> >
> > (1) This is a type-safe construct, and
> > (2) This is sufficient to catch all errors that originate from within
> > the library, regardless of their exact type.
> >
> > Primarily, it's a ploy to get the SourceInfo stuff out of the
> > common/root exception for type safety reasons.
> >
> > (Later in the series, you'll see there's a few places where we try to
> > fake SourceInfo stuff in order to use QAPIError, by shuffling this
> > around, we allow ourselves to raise exceptions that don't need this
> > hackery.)
>
> I think you're conflating a real issue with a non-issue.
>
> The real issue: you want to get rid of fake QAPISourceInfo.  This isn't
> terribly important, but small cleanups count, too.  I can't see the "few
> places where we try to fake" in this series, though.  Is it in a later
> part, or am I just blind?

I was mistaken, we don't fudge it except in one place, and that gets 
fixed in the parser.py series, not this one.

What I really wanted: I wanted to make the base error object something 
that doesn't have an info field at all, fake or not, so that it can be 
ubiquitously re-used as an abstract, common ancestor.

A separate issue: Sometimes, we want to raise errors that *usually* have 
source information, but *might* not sometimes, because of reasons.

So, I guess I don't really have a particularly strong technical 
justification for this anymore, beyond "following a pattern I see and 
use in other projects":

https://docs.python.org/3/tutorial/errors.html#user-defined-exceptions

"When creating a module that can raise several distinct errors, a common 
practice is to create a base class for exceptions defined by that 
module, and subclass that to create specific exception classes for 
different error conditions"

Which QAPIError does not violate, though I usually see this pattern used 
with a tabula rasa class to maximize re-use.

Some of my parser.py drafts that attempted to split out QAPIDoc using 
the Exception-chaining method we discussed on call winds up using this 
abstract class more directly, but we aren't sure we want that yet. (Or, 
we're fairly sure we want to try to delay thinking about it. I am still 
working on re-cleaning part 5.)

> The non-issue: wanting a common base class.  Yes, we want one, but we
> already got one: QAPIError.
>
> I think the commit message should explain the real issue more clearly,
> without confusing readers with the non-issue.
>
> Makes sense?

I'll spend a few minutes and see if dropping this patch doesn't deeply 
disturb later patches (or if it can be shuffled backwards to a point 
where it makes more sense contextually).

I genuinely can't remember if it's going to wrench something else up 
later or not right now, though; and I still haven't finished rebasing 
part 5 so I don't have a "finished" product repository to test on anymore.

--js