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

[John Snow]

On 2/25/21 9:23 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: 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.

I will admit to that being true.

Do you think it's still worth the change? I do need to get rid of the 
comparison against "[]", the rest was just "Ah, while I'm here, ..." and 
I thought it was nice to get rid of the nested function.

(I think it's still worth it.)

> > +
> > +    for element in ifcond:
>
> @element is rather long.  If you hate @elt, what about @ifc?

Hate's a strong word, It just wasn't obvious to me at the time. I 
decided to expand it to what you said it stood for.

I can undo that if you are attached to 'elt', but I don't share the view 
that 'element' is somehow burdensomely long.

> > +        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 == []:

Should be "if not ifcond", though I suppose pylint does not actually 
whine about this. Guido lives in my head rent-free.

>               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]

You know, I didn't actually know this worked in Python, because I didn't 
think "x = 3" was an expression that had a value. I thought that was the 
entire reason they added the := operator in Python 3.8.

Neat, I guess.

> +
> +    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.

Looks OK, I'll test with it.