Re: [PATCH 15/16] qapi/expr.py: move related checks inside check_xxx functions

[Eduardo Habkost]

On Tue, Sep 22, 2020 at 05:13:12PM -0400, John Snow wrote:
> There's not a big obvious difference between the types of checks that
> happen in the main function versus the kind that happen in the
> functions. Now they're in one place for each of the main types.
> 
> As part of the move, spell out the required and optional keywords so
> they're obvious at a glance.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/expr.py | 55 ++++++++++++++++++++++++++------------------
>  1 file changed, 33 insertions(+), 22 deletions(-)
> 
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 69ee9e3f18..74b2681ef8 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -333,6 +333,10 @@ def check_enum(expr: Expression, info: QAPISourceInfo) 
> -> None:
>      :param expr: `Expression` to validate.
>      :param info: QAPI source file information.
>      """
> +    check_keys(expr, info, 'enum',
> +               required=('enum', 'data'),
> +               optional=('if', 'features', 'prefix'))
> +
>      name = expr['enum']
>      members = expr['data']
>      prefix = expr.get('prefix')
> @@ -363,6 +367,11 @@ def check_struct(expr: Expression, info: QAPISourceInfo) 
> -> None:
>      :param expr: `Expression` to validate.
>      :param info: QAPI source file information.
>      """
> +    check_keys(expr, info, 'struct',
> +               required=('struct', 'data'),
> +               optional=('base', 'if', 'features'))
> +    normalize_members(expr['data'])
> +
>      name = cast(str, expr['struct'])  # Asserted in check_exprs
>      members = expr['data']
>  
> @@ -377,6 +386,13 @@ def check_union(expr: Expression, info: QAPISourceInfo) 
> -> None:
>      :param expr: `Expression` to validate.
>      :param info: QAPI source file information.
>      """
> +    check_keys(expr, info, 'union',
> +               required=('union', 'data'),
> +               optional=('base', 'discriminator', 'if', 'features'))
> +
> +    normalize_members(expr.get('base'))
> +    normalize_members(expr['data'])
> +
>      name = cast(str, expr['union'])  # Asserted in check_exprs
>      base = expr.get('base')
>      discriminator = expr.get('discriminator')
> @@ -409,6 +425,11 @@ def check_alternate(expr: Expression, info: 
> QAPISourceInfo) -> None:
>      :param expr: Expression to validate.
>      :param info: QAPI source file information.
>      """
> +    check_keys(expr, info, 'alternate',
> +               required=('alternate', 'data'),
> +               optional=('if', 'features'))
> +    normalize_members(expr['data'])
> +
>      members = expr['data']
>  
>      if not members:
> @@ -432,6 +453,13 @@ def check_command(expr: Expression, info: 
> QAPISourceInfo) -> None:
>      :param expr: `Expression` to validate.
>      :param info: QAPI source file information.
>      """
> +    check_keys(expr, info, 'command',
> +               required=['command'],
> +               optional=('data', 'returns', 'boxed', 'if', 'features',
> +                         'gen', 'success-response', 'allow-oob',
> +                         'allow-preconfig'))
> +    normalize_members(expr.get('data'))
> +
>      args = expr.get('data')
>      rets = expr.get('returns')
>      boxed = expr.get('boxed', False)
> @@ -453,6 +481,11 @@ def check_event(expr: Expression, info: QAPISourceInfo) 
> -> None:
>          if:       `Optional[Ifcond]`
>          features: `Optional[Features]`
>      """
> +    normalize_members(expr.get('data'))
> +    check_keys(expr, info, 'event',
> +               required=['event'],
> +               optional=('data', 'boxed', 'if', 'features'))

Why is the order reversed here?  The other functions call
check_keys() before normalize_members().


> +
>      args = expr.get('data')
>      boxed = expr.get('boxed', False)
>  
> @@ -519,38 +552,16 @@ def check_exprs(exprs: List[_JSObject]) -> 
> List[_JSObject]:
>                                 "documentation comment required")
>  
>          if meta == 'enum':
> -            check_keys(expr, info, meta,
> -                       ['enum', 'data'], ['if', 'features', 'prefix'])
>              check_enum(expr, info)
>          elif meta == 'union':
> -            check_keys(expr, info, meta,
> -                       ['union', 'data'],
> -                       ['base', 'discriminator', 'if', 'features'])
> -            normalize_members(expr.get('base'))
> -            normalize_members(expr['data'])
>              check_union(expr, info)
>          elif meta == 'alternate':
> -            check_keys(expr, info, meta,
> -                       ['alternate', 'data'], ['if', 'features'])
> -            normalize_members(expr['data'])
>              check_alternate(expr, info)
>          elif meta == 'struct':
> -            check_keys(expr, info, meta,
> -                       ['struct', 'data'], ['base', 'if', 'features'])
> -            normalize_members(expr['data'])
>              check_struct(expr, info)
>          elif meta == 'command':
> -            check_keys(expr, info, meta,
> -                       ['command'],
> -                       ['data', 'returns', 'boxed', 'if', 'features',
> -                        'gen', 'success-response', 'allow-oob',
> -                        'allow-preconfig'])
> -            normalize_members(expr.get('data'))
>              check_command(expr, info)
>          elif meta == 'event':
> -            check_keys(expr, info, meta,
> -                       ['event'], ['data', 'boxed', 'if', 'features'])
> -            normalize_members(expr.get('data'))
>              check_event(expr, info)
>          else:
>              assert False, 'unexpected meta type'
> -- 
> 2.26.2
> 

-- 
Eduardo