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

[Markus Armbruster]

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?

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?