Re: [PATCH] scripts/qapi: minor delinting

From: John Snow <jsnow@redhat.com>
To: Markus Armbruster <armbru@redhat.com>
Cc: Michael Roth <michael.roth@amd.com>, qemu-devel <qemu-devel@nongnu.org>
Subject: Re: [PATCH] scripts/qapi: minor delinting
Date: Fri, 11 Feb 2022 13:36:02 -0500	[thread overview]
Message-ID: <CAFn=p-ZVqhGF=6SEYtD4F=p0iyyWK+6oNvc_w-7ctkmEmpOP9w@mail.gmail.com> (raw)
In-Reply-To: <CAFn=p-ZcCjx5eeVcZm0fnc_bzFoffx_GGnCAgQKngGcOXrFLBw@mail.gmail.com>

On Fri, Feb 11, 2022 at 12:11 PM John Snow <jsnow@redhat.com> wrote:
>
> On Fri, Feb 11, 2022 at 6:58 AM Markus Armbruster <armbru@redhat.com> wrote:
> >
> > 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?
> >
>
> I've been growing that good-names list out from python/, so they
> probably appear over in there somewhere. And uh, eh. In the interest
> of just moving on to something more interesting, sure. (I reserve the
> right to change my mind when I attempt to get the CI system protecting
> the type safety of qapi-gen, when we'll need a single configuration
> for everything. We can fight about it then.)

(Oh, and some of the names are the default allow list, actually.
Setting the variable overwrites the default configuration instead of
amending it, so that's where some of those come from, too. My goal is
generally to stick to as close to the default configuration as I can
bear, in the hopes of preventing the fostering of a coding style that
is too specialized to just qemu.git.)