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

[Markus Armbruster]

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?

> +                 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>