On 4/16/21 2:04 AM, Markus Armbruster wrote:
John Snow <js...@redhat.com> writes:
On 4/15/21 2:44 AM, Markus Armbruster wrote:
John Snow <js...@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
