marcandre.lureau@redhat.com writes:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Replace the simple list sugar form with a recursive structure that will
> accept other operators in the following commits (all, any or not).
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> scripts/qapi/common.py | 23 +++++--
> scripts/qapi/expr.py | 52 ++++++++++------
> scripts/qapi/schema.py | 2 +-
> tests/qapi-schema/bad-if-empty-list.json | 2 +-
> tests/qapi-schema/bad-if-list.json | 2 +-
> tests/qapi-schema/bad-if.err | 3 +-
> tests/qapi-schema/doc-good.json | 3 +-
> tests/qapi-schema/doc-good.out | 13 ++--
> tests/qapi-schema/doc-good.txt | 6 ++
> tests/qapi-schema/enum-if-invalid.err | 3 +-
> tests/qapi-schema/features-if-invalid.err | 2 +-
> tests/qapi-schema/qapi-schema-test.json | 25 ++++----
> tests/qapi-schema/qapi-schema-test.out | 62 +++++++++----------
> .../qapi-schema/struct-member-if-invalid.err | 2 +-
> .../qapi-schema/union-branch-if-invalid.json | 2 +-
> 15 files changed, 119 insertions(+), 83 deletions(-)
>
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 5181a0f167..51463510c9 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -13,7 +13,8 @@
>
> import re
> from typing import (
> - List,
> + Any,
> + Dict,
> Match,
> Optional,
> Union,
> @@ -199,16 +200,28 @@ def guardend(name: str) -> str:
> name=c_fname(name).upper())
>
>
> -def cgen_ifcond(ifcond: Union[str, List[str]]) -> str:
> +def docgen_ifcond(ifcond: Union[str, Dict[str, Any]]) -> str:
Uh, why do you swap cgen_ifcond() and docgen_ifcond()? Accident?
> if not ifcond:
> return ''
> - return '(' + ') && ('.join(ifcond) + ')'
> + if isinstance(ifcond, str):
> + return ifcond
>
> + oper, operands = next(iter(ifcond.items()))
> + oper = {'all': ' and '}[oper]
> + operands = [docgen_ifcond(o) for o in operands]
> + return '(' + oper.join(operands) + ')'
What a nice review speedbump you buried here...
The whole block boils down to the much less exciting
operands = [docgen_ifcond(o) for o in ifcond['all']]
return '(' + ' and '.join(operands) + ')'
Peeking ahead, I understand that you did it this way here so you can
extend it trivially there. Matter of taste; what counts is the final
result and minimizing reviewer WTFs/minute along the way.
Since the WTFs/minute is a done deed now, what remains is the final
result, which I expect to review shortly. But please try a bit harder
to be boring next time ;)
>
> -def docgen_ifcond(ifcond: Union[str, List[str]]) -> str:
> +
> +def cgen_ifcond(ifcond: Union[str, Dict[str, Any]]) -> str:
> if not ifcond:
> return ''
> - return ' and '.join(ifcond)
> + if isinstance(ifcond, str):
> + return ifcond
This is what gets rid of the redundant parenthesises in the common case
"single condition string".
> +
> + oper, operands = next(iter(ifcond.items()))
> + oper = {'all': '&&'}[oper]
> + operands = [cgen_ifcond(o) for o in operands]
> + return '(' + (') ' + oper + ' (').join(operands) + ')'
This line is hard to read. Easier, I think:
oper = {'all': ' && '}[oper]
operands = ['(' + cgen_ifcond(o) + ')' for o in operands]
return oper.join(operands)
Neither your version nor mine gets rid of the redundant parenthesises in
the (uncommon) case "complex condition expression".
tests/test-qapi-introspect.c still has
#if (defined(TEST_IF_COND_1)) && (defined(TEST_IF_COND_2))
QLIT_QSTR("feature1"),
#endif /* (defined(TEST_IF_COND_1)) && (defined(TEST_IF_COND_2)) */
Mildly annoying. I'm willing to leave this for later.
Code smell: cgen_ifcond() and docgen_ifcond() are almost identical. Can
also be left for later.
>
>
> def gen_if(cond: str) -> str:
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 496f7e0333..3ee66c5f62 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -259,14 +259,12 @@ def check_flags(expr: _JSONObject, info: QAPISourceInfo) -> None:
>
> def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None:
> """
> - Normalize and validate the ``if`` member of an object.
> + Validate the ``if`` member of an object.
>
> - The ``if`` member may be either a ``str`` or a ``List[str]``.
> - A ``str`` value will be normalized to ``List[str]``.
> + The ``if`` member may be either a ``str`` or a dict.
>
> :forms:
> - :sugared: ``Union[str, List[str]]``
> - :canonical: ``List[str]``
> + :canonical: ``Union[str, dict]``
Does this :forms: thing make sense without any :sugared:? John, you
added (invented?) it in commit a48653638fa, but no explanation made it
into the tree.
This is just a "field list" ... it's just markup that renders like a bulleted definition list kind of thing. The :field list: syntax is useful only so far as we use it consistently; does it make sense without at least 2 entries? it CAN, if by analogy with the other docstrings. It's just a visual consistency thing, it doesn't have any special meaning.
i.e. unlike the other field lists (param, return, raise) it has no special recognition by the Sphinx python domain.
I won't push very hard for having it be kept either way, though Union[str, dict] is kind of a cop-out and doesn't actually convey the concrete form, which was the intent of adding these in the first place.
--js