[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 7/7] qapi: remove JSON value FIXME
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v3 7/7] qapi: remove JSON value FIXME |
Date: |
Sat, 11 Feb 2023 07:42:22 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) |
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]
- [PATCH v3 0/7] qapi: static typing conversion, pt5c, John Snow, 2023/02/09
- [PATCH v3 2/7] qapi: update pylint configuration, John Snow, 2023/02/09
- [PATCH v3 1/7] qapi: Update flake8 config, John Snow, 2023/02/09
- [PATCH v3 5/7] qapi/parser: add QAPIExpression type, John Snow, 2023/02/09
- [PATCH v3 7/7] qapi: remove JSON value FIXME, John Snow, 2023/02/09
- Re: [PATCH v3 7/7] qapi: remove JSON value FIXME,
Markus Armbruster <=
- [PATCH v3 4/7] qapi/expr: add typing workaround for AbstractSet, John Snow, 2023/02/09
- [PATCH v3 6/7] qapi: remove _JSONObject, John Snow, 2023/02/09
- [PATCH v3 3/7] qapi/expr: Split check_expr out from check_exprs, John Snow, 2023/02/09