On Thu, Feb 10, 2022 at 10:56 AM Markus Armbruster <arm...@redhat.com> wrote:
>
> John Snow <js...@redhat.com> writes:
>
> > Just cleaning up some cobwebs.
> >
> > Signed-off-by: John Snow <js...@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 <arm...@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
