Re: [PATCH] scripts/qapi: minor delinting

[John Snow <jsnow@redhat.com>]

On Thu, Feb 10, 2022 at 10:56 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> John Snow <jsnow@redhat.com> writes:
>
> > Just cleaning up some cobwebs.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  scripts/qapi/commands.py | 2 +-
> >  scripts/qapi/events.py   | 6 +++---
> >  scripts/qapi/types.py    | 6 +++++-
> >  scripts/qapi/visit.py    | 6 +++++-
> >  4 files changed, 14 insertions(+), 6 deletions(-)
> >
> > diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> > index 869d799ed2..38ca38a7b9 100644
> > --- a/scripts/qapi/commands.py
> > +++ b/scripts/qapi/commands.py
> > @@ -25,8 +25,8 @@
> >      QAPIGenC,
> >      QAPISchemaModularCVisitor,
> >      build_params,
> > -    ifcontext,
> >      gen_special_features,
> > +    ifcontext,
> >  )
> >  from .schema import (
> >      QAPISchema,
> > diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
> > index 27b44c49f5..8edf43d8da 100644
> > --- a/scripts/qapi/events.py
> > +++ b/scripts/qapi/events.py
> > @@ -109,15 +109,15 @@ def gen_event_send(name: str,
> >          if not boxed:
> >              ret += gen_param_var(arg_type)
> >
> > -    for f in features:
> > -        if f.is_special():
> > +    for feat in features:
> > +        if feat.is_special():
> >              ret += mcgen('''
> >
> >      if (compat_policy.%(feat)s_output == COMPAT_POLICY_OUTPUT_HIDE) {
> >          return;
> >      }
> >  ''',
> > -                         feat=f.name)
> > +                         feat=feat.name)
> >
> >      ret += mcgen('''
> >
>
> Meh.  Expressive variable names are good when there's something useful
> to express.  But what's the added value in such a simple, obvious loop?
>
> Besides:
>
>     $ git-grep 'for . in' scripts/qapi | wc -l
>     42
>     $ git-grep -E 'for [A-Za-z0-9]{2,} in' scripts/qapi | wc -l
>     31
>
> > diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> > index 3013329c24..477d027001 100644
> > --- a/scripts/qapi/types.py
> > +++ b/scripts/qapi/types.py
> > @@ -16,7 +16,11 @@
> >  from typing import List, Optional
> >
> >  from .common import c_enum_const, c_name, mcgen
> > -from .gen import QAPISchemaModularCVisitor, gen_special_features, ifcontext
> > +from .gen import (
> > +    QAPISchemaModularCVisitor,
> > +    gen_special_features,
> > +    ifcontext,
> > +)
> >  from .schema import (
> >      QAPISchema,
> >      QAPISchemaEnumMember,
> > diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> > index e13bbe4292..380fa197f5 100644
> > --- a/scripts/qapi/visit.py
> > +++ b/scripts/qapi/visit.py
> > @@ -21,7 +21,11 @@
> >      indent,
> >      mcgen,
> >  )
> > -from .gen import QAPISchemaModularCVisitor, gen_special_features, ifcontext
> > +from .gen import (
> > +    QAPISchemaModularCVisitor,
> > +    gen_special_features,
> > +    ifcontext,
> > +)
> >  from .schema import (
> >      QAPISchema,
> >      QAPISchemaEnumMember,
>
> Everything else, gladly
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>

The problem with whitelisting single-letter variable names is that
it's done on a per-name basis, like allowing "x, y, z" or "i, j, k". I
could whitelist "f", "m", etc but there's no way to whitelist "for f
in features" or "for m im members" ... So admittedly, I just stuck
with the default, even though it's a little annoying. It's what I use
for python/, and I had previously used it for ./scripts/qapi/, so I
was just carrying on.

In general: I like the idea of forbidding single-letter variable names
because I prefer things to be more verbose than terse as a habit. In
practice: yeah, it's hard to strictly defend any one change as
obviously superior. I preferred "for feature in features", which you
did not like because the plural wasn't distinct enough (fair!), so I
started using "for feat in features" as a compromise.

If on third thought you don't like any of this, we can change course,
but then maybe we should also undo the other changes we already
checked in. (At this point, I feel like it's maybe less churn to
continue on the path I have been, but... you're the boss here.)

--js