Re: [PATCH v6 07/11] qapi: replace if condition list with dict {'all': [...]}

[Markus Armbruster]

One more thing...

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

[...]

> 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]``
>  
>      :param expr: The expression containing the ``if`` member to validate.
>      :param info: QAPI schema source file information.
> @@ -275,31 +273,45 @@ def check_if(expr: _JSONObject, info: QAPISourceInfo, 
> source: str) -> None:
>      :raise QAPISemError:
>          When the "if" member fails validation, or when there are no
>          non-empty conditions.
> -    :return: None, ``expr`` is normalized in-place as needed.
> +    :return: None
>      """
>      ifcond = expr.get('if')
>      if ifcond is None:
>          return
>  
> -    if isinstance(ifcond, list):
> -        if not ifcond:
> -            raise QAPISemError(
> -                info, "'if' condition [] of %s is useless" % source)
> -    else:
> -        # Normalize to a list
> -        ifcond = expr['if'] = [ifcond]
> +    def _check_if(cond: Union[str, object]) -> None:
> +        if isinstance(cond, str):
> +            if not cond.strip():
> +                raise QAPISemError(
> +                    info,
> +                    "'if' condition '%s' of %s makes no sense"
> +                    % (cond, source))
> +            return
>  
> -    for elt in ifcond:
> -        if not isinstance(elt, str):
> +        if not isinstance(cond, dict):
>              raise QAPISemError(
>                  info,
> -                "'if' condition of %s must be a string or a list of strings"
> -                % source)
> -        if not elt.strip():
> +                "'if' condition of %s must be a string or a dict" % source)
> +        if len(cond) != 1:
>              raise QAPISemError(
>                  info,
> -                "'if' condition '%s' of %s makes no sense"
> -                % (elt, source))
> +                "'if' condition dict of %s must have one key: "
> +                "'all'" % source)

Not covered by the negative tests in tests/qapi-schema/.  Please
double-check that all new errors are covered.

> +        check_keys(cond, info, "'if' condition", [],
> +                   ["all"])
> +
> +        oper, operands = next(iter(cond.items()))
> +        if not operands:
> +            raise QAPISemError(
> +                info, "'if' condition [] of %s is useless" % source)
> +
> +        if oper in ("all") and not isinstance(operands, list):
> +            raise QAPISemError(
> +                info, "'%s' condition of %s must be a list" % (oper, source))
> +        for operand in operands:
> +            _check_if(operand)
> +
> +    _check_if(ifcond)
>  
>  

[...]