Re: [PATCH v3 7/7] qapi: remove JSON value FIXME

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> With the two major JSON-ish type hierarchies clarified for distinct
> purposes; QAPIExpression for parsed expressions and JSONValue for

The comment you remove talks about _ExprValue, not QAPIExpression.

> introspection data, remove this FIXME as no longer an action item.
>
> In theory, it may be possible to define a completely agnostic
> one-size-fits-all JSON type hierarchy that any other user could borrow -
> in practice, it's tough to wrangle the differences between invariant,
> covariant and contravariant types: input and output parameters demand
> different properties of such a structure. As such, it's simply more
> trouble than it's worth.

I think there's a weightier counter-argument struggling to get out.

As I explained under v2's cover letter, the two types represent things
that are only superficially similar.

_ExprValue is the obvious stupid abstract syntax tree for the QAPI
schema language, with str and bool leaves (QAPI doesn't support
floating-point numbers), OrderedDict and list inner nodes.  It is used
for parser output.

QAPIExpression augments _ExprValue, adding a QAPISourceInfo (identifying
the expression's source) and a QAPIDoc (the expressions documentation).
It is used to represent QAPI top-level expressions.

JSONValue is an annotated JSON abstract syntax tree.  QAPIExpression and
_ExprValue are related to it only insofar as the QAPI schema language is
(rather loosely) patterned after JSON.

Moreover, the two ASTs serve different purposes.  QAPIExpression and
_ExprValue represent *input*: they are produced by a parser and consumed
by a semantic analyzer.  JSONValue represents *output*: it is produced
within a backend so we can separate the JSON text formatting aspect.

Consolidating these two ASTs makes no sense.

Suggest to work this argument into your commit message.

> So, declare this "done enough for now".

Agree.

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/parser.py | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index c165bd3912c..b5afdd703e7 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -42,10 +42,6 @@
>  _ExprValue = Union[List[object], Dict[str, object], str, bool]
>  
>  
> -# FIXME: Consolidate and centralize definitions for _ExprValue and
> -# JSONValue; currently scattered across several modules.
> -
> -
>  # 3.6 workaround: can be removed when Python 3.7+ is our required version.
>  if TYPE_CHECKING:
>      _UserDict = UserDict[str, object]