Re: [PATCH] scripts/qapi: minor delinting

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

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

There are only 26 single-letter variable names.  Whitelist them all?

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

@feat is a perfectly adequate name.  So is @f as long as the loop is
small.  What annoys me here is the churn.

Sadly, pylint's invalid-name mixes up two things: PEP-8 conventions like
"use CamelCase for class names", and its own style rules like "names
should be least three characters long".  The former is easy to decide,
and welcome help.  The latter is actually a proxy for "use sensible
names to make the code easier to read", because pylint isn't smart
enough to judge "easy to read".  Fine, leave it to reviewers then!

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

I don't think we need to undo.

Several of the names in scripts/qapi/pylintrc's good-names don't
actually occur in the code.

Could I persuade you to replace it with something like

    good-names-rgxs=^[_a-z][_a-z0-9]?$

and call it a day?