Re: [PATCH v3 09/16] qapi/expr.py: Consolidate check_if_str calls in check_if

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> This is a small rewrite to address some minor style nits.
>
> Don't compare against the empty list to check for the empty condition, and
> move the normalization forward to unify the check on the now-normalized
> structure.
>
> With the check unified, the local nested function isn't needed anymore
> and can be brought down into the normal flow of the function. With the
> nesting level changed, shuffle the error strings around a bit to get
> them to fit in 79 columns.
>
> Note: though ifcond is typed as Sequence[str] elsewhere, we *know* that
> the parser will produce real, bona-fide lists. It's okay to check
> isinstance(ifcond, list) here.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/expr.py | 36 ++++++++++++++++--------------------
>  1 file changed, 16 insertions(+), 20 deletions(-)
>
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index df6c64950fa..3235a3b809e 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -128,30 +128,26 @@ def check_flags(expr: Expression, info: QAPISourceInfo) 
> -> None:
>  
>  def check_if(expr: _JSObject, info: QAPISourceInfo, source: str) -> None:
>  
> -    def check_if_str(ifcond: object) -> None:
> -        if not isinstance(ifcond, str):
> -            raise QAPISemError(
> -                info,
> -                "'if' condition of %s must be a string or a list of strings"
> -                % source)
> -        if ifcond.strip() == '':
> -            raise QAPISemError(
> -                info,
> -                "'if' condition '%s' of %s makes no sense"
> -                % (ifcond, source))
> -
>      ifcond = expr.get('if')
>      if ifcond is None:
>          return
> -    if isinstance(ifcond, list):
> -        if ifcond == []:
> +
> +    # Normalize to a list
> +    if not isinstance(ifcond, list):
> +        ifcond = [ifcond]
> +        expr['if'] = ifcond
> +
> +    if not ifcond:
> +        raise QAPISemError(info, f"'if' condition [] of {source} is useless")

In the old code, the connection between the conditional and the error
message was a bit more obvious.

> +
> +    for element in ifcond:

@element is rather long.  If you hate @elt, what about @ifc?

> +        if not isinstance(element, str):
> +            raise QAPISemError(info, (
> +                f"'if' condition of {source}"
> +                " must be a string or a list of strings"))
> +        if element.strip() == '':
>              raise QAPISemError(
> -                info, "'if' condition [] of %s is useless" % source)
> -        for elt in ifcond:
> -            check_if_str(elt)
> -    else:
> -        check_if_str(ifcond)
> -        expr['if'] = [ifcond]
> +                info, f"'if' condition '{element}' of {source} makes no 
> sense")
>  
>  
>  def normalize_members(members: object) -> None:

Perhaps:

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index df6c64950f..e904924599 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -128,30 +128,26 @@ def check_flags(expr: Expression, info: QAPISourceInfo) 
-> None:
 
 def check_if(expr: _JSObject, info: QAPISourceInfo, source: str) -> None:
 
-    def check_if_str(ifcond: object) -> None:
-        if not isinstance(ifcond, str):
-            raise QAPISemError(
-                info,
-                "'if' condition of %s must be a string or a list of strings"
-                % source)
-        if ifcond.strip() == '':
-            raise QAPISemError(
-                info,
-                "'if' condition '%s' of %s makes no sense"
-                % (ifcond, source))
-
     ifcond = expr.get('if')
     if ifcond is None:
         return
+
     if isinstance(ifcond, list):
         if ifcond == []:
             raise QAPISemError(
                 info, "'if' condition [] of %s is useless" % source)
-        for elt in ifcond:
-            check_if_str(elt)
     else:
-        check_if_str(ifcond)
-        expr['if'] = [ifcond]
+        # Normalize to a list
+        ifcond = expr['if'] = [ifcond]
+
+    for elt in ifcond:
+        if not isinstance(elt, str):
+            raise QAPISemError(info, (
+                f"'if' condition of {source}"
+                " must be a string or a list of strings"))
+        if elt.strip() == '':
+            raise QAPISemError(
+                info, f"'if' condition '{elt}' of {source} makes no sense")
 
 
 def normalize_members(members: object) -> None:


Bonus: slightly less churn.