On 3/25/21 11:15 AM, Markus Armbruster wrote:
> 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: although 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 | 32 ++++++++++++++------------------
>> 1 file changed, 14 insertions(+), 18 deletions(-)
>>
>> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
>> index ea9d39fcf2..5921fa34ab 100644
>> --- a/scripts/qapi/expr.py
>> +++ b/scripts/qapi/expr.py
>> @@ -144,30 +144,26 @@ def check_flags(expr: _JSONObject, info: QAPISourceInfo) -> None:
>>
>> def check_if(expr: _JSONObject, 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 == []:
>> + if not ifcond:
>> raise QAPISemError(
>> - info, "'if' condition [] of %s is useless" % source)
>> - for elt in ifcond:
>> - check_if_str(elt)
>> + info, f"'if' condition [] of {source} is useless")
>
> Unrelated change from interpolation to formatted string literal.
>
>> 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 not elt.strip():
>> + raise QAPISemError(
>> + info, f"'if' condition '{elt}' of {source} makes no sense")
>
> Likewise.
>
> I like formatted string literals, they're often easier to read than
> interpolation. But let's try to keep patches focused on their stated
> purpose.
>
> I'd gladly consider a series that convers to formatted strings
> wholesale. But I guess we better finish the typing job, first.
>
I am dreaming of a lush meadow.