Rename QAPIError to QAPISourceError, and then create a new QAPIError
class that serves as the basis for all of our other custom exceptions.
Add docstrings to explain the intended function of each error class.
Signed-off-by: John Snow <jsnow@redhat.com>
---
docs/sphinx/qapidoc.py | 3 ++-
scripts/qapi/error.py | 11 +++++++++--
scripts/qapi/schema.py | 5 +++--
3 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index b7b86b5dff..458b1f477e 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -34,7 +34,8 @@
from sphinx.util.nodes import nested_parse_with_titles
import sphinx
from qapi.gen import QAPISchemaVisitor
-from qapi.schema import QAPIError, QAPISemError, QAPISchema
+from qapi.error import QAPIError, QAPISemError
+from qapi.schema import QAPISchema
# Sphinx up to 1.6 uses AutodocReporter; 1.7 and later
diff --git a/scripts/qapi/error.py b/scripts/qapi/error.py
index ae60d9e2fe..126dda7c9b 100644
--- a/scripts/qapi/error.py
+++ b/scripts/qapi/error.py
@@ -13,6 +13,11 @@
class QAPIError(Exception):
+ """Base class for all exceptions from the QAPI package."""
+
+
+class QAPISourceError(QAPIError):
+ """Error class for all exceptions identifying a source location."""
def __init__(self, info, col, msg):
Exception.__init__(self)
self.info = info
@@ -27,7 +32,8 @@ def __str__(self):
return loc + ': ' + self.msg
-class QAPIParseError(QAPIError):
+class QAPIParseError(QAPISourceError):
+ """Error class for all QAPI schema parsing errors."""
def __init__(self, parser, msg):
col = 1
for ch in parser.src[parser.line_pos:parser.pos]:
@@ -38,6 +44,7 @@ def __init__(self, parser, msg):
super().__init__(parser.info, col, msg)
-class QAPISemError(QAPIError):
+class QAPISemError(QAPISourceError):
+ """Error class for semantic QAPI errors."""
def __init__(self, info, msg):
super().__init__(info, None, msg)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 01cdd753cd..1849c3e45f 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -20,7 +20,7 @@
from typing import Optional
from .common import POINTER_SUFFIX, c_name
-from .error import QAPIError, QAPISemError
+from .error import QAPISemError, QAPISourceError
from .expr import check_exprs
from .parser import QAPISchemaParser
@@ -878,7 +878,8 @@ def _def_entity(self, ent):
other_ent = self._entity_dict.get(ent.name)
if other_ent:
if other_ent.info:
- where = QAPIError(other_ent.info, None, "previous definition")
+ where = QAPISourceError(other_ent.info, None,
+ "previous definition")
raise QAPISemError(
ent.info,
"'%s' is already defined\n%s" % (ent.name, where))
--
2.30.2
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? > Add docstrings to explain the intended function of each error class. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > docs/sphinx/qapidoc.py | 3 ++- > scripts/qapi/error.py | 11 +++++++++-- > scripts/qapi/schema.py | 5 +++-- > 3 files changed, 14 insertions(+), 5 deletions(-) > > diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py > index b7b86b5dff..458b1f477e 100644 > --- a/docs/sphinx/qapidoc.py > +++ b/docs/sphinx/qapidoc.py > @@ -34,7 +34,8 @@ > from sphinx.util.nodes import nested_parse_with_titles > import sphinx > from qapi.gen import QAPISchemaVisitor > -from qapi.schema import QAPIError, QAPISemError, QAPISchema > +from qapi.error import QAPIError, QAPISemError > +from qapi.schema import QAPISchema > > > # Sphinx up to 1.6 uses AutodocReporter; 1.7 and later > diff --git a/scripts/qapi/error.py b/scripts/qapi/error.py > index ae60d9e2fe..126dda7c9b 100644 > --- a/scripts/qapi/error.py > +++ b/scripts/qapi/error.py > @@ -13,6 +13,11 @@ > > > class QAPIError(Exception): > + """Base class for all exceptions from the QAPI package.""" > + > + > +class QAPISourceError(QAPIError): > + """Error class for all exceptions identifying a source location.""" > def __init__(self, info, col, msg): > Exception.__init__(self) > self.info = info > @@ -27,7 +32,8 @@ def __str__(self): > return loc + ': ' + self.msg > > > -class QAPIParseError(QAPIError): > +class QAPIParseError(QAPISourceError): > + """Error class for all QAPI schema parsing errors.""" > def __init__(self, parser, msg): > col = 1 > for ch in parser.src[parser.line_pos:parser.pos]: > @@ -38,6 +44,7 @@ def __init__(self, parser, msg): > super().__init__(parser.info, col, msg) > > > -class QAPISemError(QAPIError): > +class QAPISemError(QAPISourceError): > + """Error class for semantic QAPI errors.""" > def __init__(self, info, msg): > super().__init__(info, None, msg) > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > index 01cdd753cd..1849c3e45f 100644 > --- a/scripts/qapi/schema.py > +++ b/scripts/qapi/schema.py > @@ -20,7 +20,7 @@ > from typing import Optional > > from .common import POINTER_SUFFIX, c_name > -from .error import QAPIError, QAPISemError > +from .error import QAPISemError, QAPISourceError > from .expr import check_exprs > from .parser import QAPISchemaParser > > @@ -878,7 +878,8 @@ def _def_entity(self, ent): > other_ent = self._entity_dict.get(ent.name) > if other_ent: > if other_ent.info: > - where = QAPIError(other_ent.info, None, "previous definition") > + where = QAPISourceError(other_ent.info, None, > + "previous definition") > raise QAPISemError( > ent.info, > "'%s' is already defined\n%s" % (ent.name, where))
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.)
>> Add docstrings to explain the intended function of each error class.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> docs/sphinx/qapidoc.py | 3 ++-
>> scripts/qapi/error.py | 11 +++++++++--
>> scripts/qapi/schema.py | 5 +++--
>> 3 files changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
>> index b7b86b5dff..458b1f477e 100644
>> --- a/docs/sphinx/qapidoc.py
>> +++ b/docs/sphinx/qapidoc.py
>> @@ -34,7 +34,8 @@
>> from sphinx.util.nodes import nested_parse_with_titles
>> import sphinx
>> from qapi.gen import QAPISchemaVisitor
>> -from qapi.schema import QAPIError, QAPISemError, QAPISchema
>> +from qapi.error import QAPIError, QAPISemError
>> +from qapi.schema import QAPISchema
>>
>>
>> # Sphinx up to 1.6 uses AutodocReporter; 1.7 and later
>> diff --git a/scripts/qapi/error.py b/scripts/qapi/error.py
>> index ae60d9e2fe..126dda7c9b 100644
>> --- a/scripts/qapi/error.py
>> +++ b/scripts/qapi/error.py
>> @@ -13,6 +13,11 @@
>>
>>
>> class QAPIError(Exception):
>> + """Base class for all exceptions from the QAPI package."""
>> +
>> +
>> +class QAPISourceError(QAPIError):
>> + """Error class for all exceptions identifying a source location."""
>> def __init__(self, info, col, msg):
>> Exception.__init__(self)
>> self.info = info
>> @@ -27,7 +32,8 @@ def __str__(self):
>> return loc + ': ' + self.msg
>>
>>
>> -class QAPIParseError(QAPIError):
>> +class QAPIParseError(QAPISourceError):
>> + """Error class for all QAPI schema parsing errors."""
>> def __init__(self, parser, msg):
>> col = 1
>> for ch in parser.src[parser.line_pos:parser.pos]:
>> @@ -38,6 +44,7 @@ def __init__(self, parser, msg):
>> super().__init__(parser.info, col, msg)
>>
>>
>> -class QAPISemError(QAPIError):
>> +class QAPISemError(QAPISourceError):
>> + """Error class for semantic QAPI errors."""
>> def __init__(self, info, msg):
>> super().__init__(info, None, msg)
>> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
>> index 01cdd753cd..1849c3e45f 100644
>> --- a/scripts/qapi/schema.py
>> +++ b/scripts/qapi/schema.py
>> @@ -20,7 +20,7 @@
>> from typing import Optional
>>
>> from .common import POINTER_SUFFIX, c_name
>> -from .error import QAPIError, QAPISemError
>> +from .error import QAPISemError, QAPISourceError
>> from .expr import check_exprs
>> from .parser import QAPISchemaParser
>>
>> @@ -878,7 +878,8 @@ def _def_entity(self, ent):
>> other_ent = self._entity_dict.get(ent.name)
>> if other_ent:
>> if other_ent.info:
>> - where = QAPIError(other_ent.info, None, "previous definition")
>> + where = QAPISourceError(other_ent.info, None,
>> + "previous definition")
>> raise QAPISemError(
>> ent.info,
>> "'%s' is already defined\n%s" % (ent.name, where))
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?
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
© 2016 - 2026 Red Hat, Inc.