* [PATCH] scripts/qapi: minor delinting @ 2022-02-04 22:52 John Snow 2022-02-10 15:56 ` Markus Armbruster 0 siblings, 1 reply; 7+ messages in thread From: John Snow @ 2022-02-04 22:52 UTC (permalink / raw) To: qemu-devel; +Cc: Michael Roth, John Snow, Markus Armbruster 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(''' 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, -- 2.34.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] scripts/qapi: minor delinting 2022-02-04 22:52 [PATCH] scripts/qapi: minor delinting John Snow @ 2022-02-10 15:56 ` Markus Armbruster 2022-02-10 17:17 ` John Snow 0 siblings, 1 reply; 7+ messages in thread From: Markus Armbruster @ 2022-02-10 15:56 UTC (permalink / raw) To: John Snow; +Cc: Michael Roth, qemu-devel 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] scripts/qapi: minor delinting 2022-02-10 15:56 ` Markus Armbruster @ 2022-02-10 17:17 ` John Snow 2022-02-11 11:58 ` Markus Armbruster 0 siblings, 1 reply; 7+ messages in thread From: John Snow @ 2022-02-10 17:17 UTC (permalink / raw) To: Markus Armbruster; +Cc: Michael Roth, qemu-devel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] scripts/qapi: minor delinting 2022-02-10 17:17 ` John Snow @ 2022-02-11 11:58 ` Markus Armbruster 2022-02-11 17:11 ` John Snow 0 siblings, 1 reply; 7+ messages in thread From: Markus Armbruster @ 2022-02-11 11:58 UTC (permalink / raw) To: John Snow; +Cc: Michael Roth, qemu-devel 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? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] scripts/qapi: minor delinting 2022-02-11 11:58 ` Markus Armbruster @ 2022-02-11 17:11 ` John Snow 2022-02-11 18:36 ` John Snow 0 siblings, 1 reply; 7+ messages in thread From: John Snow @ 2022-02-11 17:11 UTC (permalink / raw) To: Markus Armbruster; +Cc: Michael Roth, qemu-devel 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.) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] scripts/qapi: minor delinting 2022-02-11 17:11 ` John Snow @ 2022-02-11 18:36 ` John Snow 2022-02-14 13:48 ` Markus Armbruster 0 siblings, 1 reply; 7+ messages in thread From: John Snow @ 2022-02-11 18:36 UTC (permalink / raw) To: Markus Armbruster; +Cc: Michael Roth, qemu-devel 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.) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] scripts/qapi: minor delinting 2022-02-11 18:36 ` John Snow @ 2022-02-14 13:48 ` Markus Armbruster 0 siblings, 0 replies; 7+ messages in thread From: Markus Armbruster @ 2022-02-14 13:48 UTC (permalink / raw) To: John Snow; +Cc: Michael Roth, qemu-devel John Snow <jsnow@redhat.com> writes: > 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: [...] >> > 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. I see. >> 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.) Okay. > (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.) I agree we shouldn't invent a Python coding style. I just wish pylint didn't, either ;-P ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-02-14 14:21 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-02-04 22:52 [PATCH] scripts/qapi: minor delinting John Snow 2022-02-10 15:56 ` Markus Armbruster 2022-02-10 17:17 ` John Snow 2022-02-11 11:58 ` Markus Armbruster 2022-02-11 17:11 ` John Snow 2022-02-11 18:36 ` John Snow 2022-02-14 13:48 ` Markus Armbruster
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).