On Wed, Feb 15, 2023 at 4:43 AM Markus Armbruster <arm...@redhat.com> wrote:
>
> John Snow <js...@redhat.com> writes:
>
> > This patch creates a new type, QAPIExpression, which represents a parsed
> > expression complete with QAPIDoc and QAPISourceInfo.
> >
> > This patch turns parser.exprs into a list of QAPIExpression instead,
> > and adjusts expr.py to match.
> >
> > This allows the types we specify in parser.py to be "remembered" all the
> > way through expr.py and into schema.py. Several assertions around
> > packing and unpacking this data can be removed as a result.
>
> Suggest to add:
>
> It also corrects a harmless typing error. Before the patch,
> check_exprs() allegedly takes a List[_JSONObject]. It actually takes
> a list of dicts of the form
>
> {'expr': E, 'info': I, 'doc': D}
>
> where E is of type _ExprValue, I is of type QAPISourceInfo, and D is
> of type QAPIDoc. Key 'doc' is optional. This is not a _JSONObject!
> Passes type checking anyway, because _JSONObject is Dict[str, object].
>
> > Signed-off-by: John Snow <js...@redhat.com>
>
> [...]
>
> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> > index 1b006cdc133..50906e27d49 100644
> > --- a/scripts/qapi/parser.py
> > +++ b/scripts/qapi/parser.py
> > @@ -21,6 +21,7 @@
> > TYPE_CHECKING,
> > Dict,
> > List,
> > + Mapping,
> > Optional,
> > Set,
> > Union,
> > @@ -37,15 +38,24 @@
> > from .schema import QAPISchemaFeature, QAPISchemaMember
> >
> >
> > -#: Represents a single Top Level QAPI schema expression.
> > -TopLevelExpr = Dict[str, object]
> > -
> > # Return value alias for get_expr().
> > _ExprValue = Union[List[object], Dict[str, object], str, bool]
> >
> > -# FIXME: Consolidate and centralize definitions for TopLevelExpr,
> > -# _ExprValue, _JSONValue, and _JSONObject; currently scattered across
> > -# several modules.
> > +
> > +# FIXME: Consolidate and centralize definitions for _ExprValue,
> > +# JSONValue, and _JSONObject; currently scattered across several
> > +# modules.
> > +
> > +
> > +class QAPIExpression(Dict[str, object]):
> > + # pylint: disable=too-few-public-methods
> > + def __init__(self,
> > + data: Mapping[str, object],
>
> Would @expr be a better name?
>
Linters don't seem to mind the new parameter name. Feel free to make
the substitution if you don't mind "expr" sometimes referring to a
QAPIExpression and sometimes referring to the JSON-y data inside of
it. I am less particular about consistency in my local variable names
than you are, so it's a matter of taste for you specifically.
> > + info: QAPISourceInfo,
> > + doc: Optional['QAPIDoc'] = None):
> > + super().__init__(data)
> > + self.info = info
> > + self.doc: Optional['QAPIDoc'] = doc
> >
> >
> > class QAPIParseError(QAPISourceError):
>
> [...]
>
> Regardless,
> Reviewed-by: Markus Armbruster <arm...@redhat.com>
>
