John Snow <js...@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 <js...@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.
