Re: [PATCH v4 4/6] qapi/parser: add QAPIExpression type

[John Snow]

On Wed, Feb 15, 2023 at 4:43 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> John Snow <jsnow@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 <jsnow@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 <armbru@redhat.com>
>