diff mbox series

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

Message ID 20200922211313.4082880-16-jsnow@redhat.com (mailing list archive)
State New, archived
Headers show
Series qapi: static typing conversion, pt2 | expand

Commit Message

John Snow Sept. 22, 2020, 9:13 p.m. UTC 
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(-)

Comments

Eduardo Habkost Sept. 23, 2020, 8:25 p.m. UTC | #1 
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
>
John Snow Sept. 25, 2020, 12:58 a.m. UTC | #2 
On 9/23/20 4:25 PM, Eduardo Habkost wrote:
> 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().
> 

Oops! This was from an earlier experiment. I am dabbling with factoring 
out all of the normalization into a step that occurs discretely before 
data shape validation.

I have deep, ulterior reasons for doing this.

...But they shouldn't have leaked into this patch series, so I'll fix 
that. Thanks!

--js
Cleber Rosa Sept. 25, 2020, 1:09 a.m. UTC | #3 
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>

Reviewed-by: Cleber Rosa <crosa@redhat.com>
diff mbox series

Patch

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'))
+
     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'