| Message ID | 20200227144531.24309-5-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On 2/27/20 9:45 AM, Markus Armbruster wrote: > Signed-off-by: Markus Armbruster <armbru@redhat.com> I wrote some pylint cleanup for iotests recently, too. Are you targeting a subset of pylint errors to clean here? (Do any files pass 100%?) Consider checking in a pylintrc file that lets others run the same subset of pylint tests as you are doing so that we can prevent future regressions. Take a peek at [PATCH v6 0/9] iotests: use python logging Thanks for this series. I had a very similar series sitting waiting to go out, but this goes further in a few places. --js > --- > scripts/qapi/commands.py | 2 +- > scripts/qapi/expr.py | 3 +-- > scripts/qapi/gen.py | 9 ++++++--- > scripts/qapi/introspect.py | 2 -- > scripts/qapi/parser.py | 6 ++---- > scripts/qapi/schema.py | 9 ++++----- > 6 files changed, 14 insertions(+), 17 deletions(-) > > diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py > index 8bb6316061..0e13e82989 100644 > --- a/scripts/qapi/commands.py > +++ b/scripts/qapi/commands.py > @@ -274,7 +274,7 @@ class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor): > > void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds); > ''', > - c_prefix=c_name(self._prefix, protect=False))) > + c_prefix=c_name(self._prefix, protect=False))) > self._genc.preamble_add(mcgen(''' > #include "qemu/osdep.h" > #include "%(prefix)sqapi-commands.h" > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py > index d7a289eded..fecf466fa7 100644 > --- a/scripts/qapi/expr.py > +++ b/scripts/qapi/expr.py > @@ -35,7 +35,6 @@ def check_name_is_str(name, info, source): > def check_name_str(name, info, source, > allow_optional=False, enum_member=False, > permit_upper=False): > - global valid_name > membername = name > > if allow_optional and name.startswith('*'): > @@ -249,7 +248,7 @@ def check_union(expr, info): > def check_alternate(expr, info): > members = expr['data'] > > - if len(members) == 0: > + if not members: > raise QAPISemError(info, "'data' must not be empty") > for (key, value) in members.items(): > source = "'data' member '%s'" % key > diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py > index e17354392b..33690bfa3b 100644 > --- a/scripts/qapi/gen.py > +++ b/scripts/qapi/gen.py > @@ -45,10 +45,10 @@ class QAPIGen: > > def write(self, output_dir): > pathname = os.path.join(output_dir, self.fname) > - dir = os.path.dirname(pathname) > - if dir: > + odir = os.path.dirname(pathname) > + if odir: > try: > - os.makedirs(dir) > + os.makedirs(odir) > except os.error as e: > if e.errno != errno.EEXIST: > raise > @@ -261,6 +261,9 @@ class QAPISchemaModularCVisitor(QAPISchemaVisitor): > genc.write(output_dir) > genh.write(output_dir) > > + def _begin_system_module(self, name): > + pass > + > def _begin_user_module(self, name): > pass > > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py > index 0cc655fd9f..b5537eddc0 100644 > --- a/scripts/qapi/introspect.py > +++ b/scripts/qapi/introspect.py > @@ -10,8 +10,6 @@ This work is licensed under the terms of the GNU GPL, version 2. > See the COPYING file in the top-level directory. > """ > > -import string > - > from qapi.common import * > from qapi.gen import QAPISchemaMonolithicCVisitor > from qapi.schema import (QAPISchemaArrayType, QAPISchemaBuiltinType, > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py > index 340f7c4633..abadacbb0e 100644 > --- a/scripts/qapi/parser.py > +++ b/scripts/qapi/parser.py > @@ -282,8 +282,7 @@ class QAPISchemaParser: > doc.end_comment() > self.accept() > return doc > - else: > - doc.append(self.val) > + doc.append(self.val) > self.accept(False) > > raise QAPIParseError(self, "documentation comment must end with '##'") > @@ -492,7 +491,7 @@ class QAPIDoc: > raise QAPIParseError(self._parser, > "'%s' can't follow '%s' section" > % (name, self.sections[0].name)) > - elif self._is_section_tag(name): > + if self._is_section_tag(name): > line = line[len(name)+1:] > self._start_section(name[:-1]) > > @@ -556,7 +555,6 @@ class QAPIDoc: > raise QAPISemError(feature.info, > "feature '%s' lacks documentation" > % feature.name) > - self.features[feature.name] = QAPIDoc.ArgSection(feature.name) > self.features[feature.name].connect(feature) > > def check_expr(self, expr): > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > index e132442c04..cfbb9758c4 100644 > --- a/scripts/qapi/schema.py > +++ b/scripts/qapi/schema.py > @@ -19,7 +19,7 @@ import re > from collections import OrderedDict > > from qapi.common import c_name, pointer_suffix > -from qapi.error import QAPIError, QAPIParseError, QAPISemError > +from qapi.error import QAPIError, QAPISemError > from qapi.expr import check_exprs > from qapi.parser import QAPISchemaParser > > @@ -96,14 +96,14 @@ class QAPISchemaVisitor: > def visit_end(self): > pass > > - def visit_module(self, fname): > + def visit_module(self, name): > pass > > def visit_needed(self, entity): > # Default to visiting everything > return True > > - def visit_include(self, fname, info): > + def visit_include(self, name, info): > pass > > def visit_builtin_type(self, name, info, json_type): > @@ -576,7 +576,7 @@ class QAPISchemaObjectTypeVariants: > assert self.tag_member.ifcond == [] > if self._tag_name: # flat union > # branches that are not explicitly covered get an empty type > - cases = set([v.name for v in self.variants]) > + cases = {v.name for v in self.variants} > for m in self.tag_member.type.members: > if m.name not in cases: > v = QAPISchemaObjectTypeVariant(m.name, self.info, > @@ -1098,7 +1098,6 @@ class QAPISchema: > > def visit(self, visitor): > visitor.visit_begin(self) > - module = None > for mod in self._module_dict.values(): > mod.visit(visitor) > visitor.visit_end() >
John Snow <jsnow@redhat.com> writes: > On 2/27/20 9:45 AM, Markus Armbruster wrote: >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > > I wrote some pylint cleanup for iotests recently, too. Are you targeting > a subset of pylint errors to clean here? > > (Do any files pass 100%?) Surely you're joking, Mr. Snow! I'm chipping away at pylint's gripes. I ran it with the following messages disabled: bad-whitespace, fixme, invalid-name, missing-docstring, too-few-public-methods, too-many-arguments, too-many-branches, too-many-instance-attributes, too-many-lines, too-many-locals, too-many-statements, unused-argument, unused-wildcard-import, These are not all obviously useless. They're just not what I want to focus on right now. Remaining: 1 x C0330: Wrong continued indentation (remove 19 spaces). Accident, will fix in v2. 8 x R0201: Method could be a function (no-self-use) Yes, but the override in a sub-class does use self. 2 x W0212: Access to a protected member _body of a client class (protected-access) Needs cleanup, but not now. 6 x W0401: Wildcard import qapi.common (wildcard-import) Not sure I care. I'd prefer not to have more wildcard imports, though. 2 x W0603: Using the global statement (global-statement) Cleanup is non-trivial. Not now. I also ran pycodestyle-3: 1 x E127 continuation line over-indented for visual indent Same as pylint's C0330, will fix in v2. 3 x E261 at least two spaces before inline comment I blame Emacs. Left for another day. 8 x E501 line too long Left for another day. 1 x E713 test for membership should be 'not in' I missed that one, will fix in v2. > Consider checking in a pylintrc file that lets others run the same > subset of pylint tests as you are doing so that we can prevent future > regressions. Working towards it, slowly. > Take a peek at [PATCH v6 0/9] iotests: use python logging > > Thanks for this series. I had a very similar series sitting waiting to > go out, but this goes further in a few places. Thanks!
On 3/4/20 3:01 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > >> On 2/27/20 9:45 AM, Markus Armbruster wrote: >>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> >> I wrote some pylint cleanup for iotests recently, too. Are you targeting >> a subset of pylint errors to clean here? >> >> (Do any files pass 100%?) > > Surely you're joking, Mr. Snow! > > I'm chipping away at pylint's gripes. I ran it with the following > messages disabled: > > bad-whitespace, > fixme, > invalid-name, > missing-docstring, > too-few-public-methods, > too-many-arguments, > too-many-branches, > too-many-instance-attributes, > too-many-lines, > too-many-locals, > too-many-statements, > unused-argument, > unused-wildcard-import, > > These are not all obviously useless. They're just not what I want to > focus on right now. > Yes, understood - so my approach is disable what I don't intend to fix, commit the pylintrc to prevent backslide, and move on. I think we have a difference in what a pylintrc means to us (the goal vs. the current status.) I didn't mean "100% without caveats", just "100% in some subset of checks". (I assume the answer is still no.) > Remaining: > > 1 x C0330: Wrong continued indentation (remove 19 spaces). > > Accident, will fix in v2. > > 8 x R0201: Method could be a function (no-self-use) > > Yes, but the override in a sub-class does use self. > > 2 x W0212: Access to a protected member _body of a client class (protected-access) > > Needs cleanup, but not now. > > 6 x W0401: Wildcard import qapi.common (wildcard-import) > > Not sure I care. I'd prefer not to have more wildcard imports, > though. > > 2 x W0603: Using the global statement (global-statement) > > Cleanup is non-trivial. Not now. > > I also ran pycodestyle-3: > > 1 x E127 continuation line over-indented for visual indent > > Same as pylint's C0330, will fix in v2. > > 3 x E261 at least two spaces before inline comment > > I blame Emacs. Left for another day. > > 8 x E501 line too long > > Left for another day. > > 1 x E713 test for membership should be 'not in' > > I missed that one, will fix in v2. > >> Consider checking in a pylintrc file that lets others run the same >> subset of pylint tests as you are doing so that we can prevent future >> regressions. > > Working towards it, slowly. > >> Take a peek at [PATCH v6 0/9] iotests: use python logging >> >> Thanks for this series. I had a very similar series sitting waiting to >> go out, but this goes further in a few places. > > Thanks! >
John Snow <jsnow@redhat.com> writes: > On 3/4/20 3:01 AM, Markus Armbruster wrote: >> John Snow <jsnow@redhat.com> writes: >> >>> On 2/27/20 9:45 AM, Markus Armbruster wrote: >>>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>> >>> I wrote some pylint cleanup for iotests recently, too. Are you targeting >>> a subset of pylint errors to clean here? >>> >>> (Do any files pass 100%?) >> >> Surely you're joking, Mr. Snow! >> >> I'm chipping away at pylint's gripes. I ran it with the following >> messages disabled: >> >> bad-whitespace, >> fixme, >> invalid-name, >> missing-docstring, >> too-few-public-methods, >> too-many-arguments, >> too-many-branches, >> too-many-instance-attributes, >> too-many-lines, >> too-many-locals, >> too-many-statements, >> unused-argument, >> unused-wildcard-import, >> >> These are not all obviously useless. They're just not what I want to >> focus on right now. >> > > Yes, understood - so my approach is disable what I don't intend to fix, > commit the pylintrc to prevent backslide, and move on. > > I think we have a difference in what a pylintrc means to us (the goal > vs. the current status.) > > I didn't mean "100% without caveats", just "100% in some subset of checks". > > (I assume the answer is still no.) To turn the answer into a yes, I'd have to disable the messages below, and some of them I'd rather keep. Tacking # pylint: disable=... to existing troublemakers may or may not be worth the ugliness (it needs to go on the same line, which almost invariably makes it awkwardly long). >> Remaining: >> >> 1 x C0330: Wrong continued indentation (remove 19 spaces). >> >> Accident, will fix in v2. >> >> 8 x R0201: Method could be a function (no-self-use) >> >> Yes, but the override in a sub-class does use self. >> >> 2 x W0212: Access to a protected member _body of a client class (protected-access) >> >> Needs cleanup, but not now. >> >> 6 x W0401: Wildcard import qapi.common (wildcard-import) >> >> Not sure I care. I'd prefer not to have more wildcard imports, >> though. >> >> 2 x W0603: Using the global statement (global-statement) >> >> Cleanup is non-trivial. Not now. >> >> I also ran pycodestyle-3: >> >> 1 x E127 continuation line over-indented for visual indent >> >> Same as pylint's C0330, will fix in v2. >> >> 3 x E261 at least two spaces before inline comment >> >> I blame Emacs. Left for another day. >> >> 8 x E501 line too long >> >> Left for another day. >> >> 1 x E713 test for membership should be 'not in' >> >> I missed that one, will fix in v2. >> >>> Consider checking in a pylintrc file that lets others run the same >>> subset of pylint tests as you are doing so that we can prevent future >>> regressions. >> >> Working towards it, slowly. >> >>> Take a peek at [PATCH v6 0/9] iotests: use python logging >>> >>> Thanks for this series. I had a very similar series sitting waiting to >>> go out, but this goes further in a few places. >> >> Thanks! >>
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py index 8bb6316061..0e13e82989 100644 --- a/scripts/qapi/commands.py +++ b/scripts/qapi/commands.py @@ -274,7 +274,7 @@ class QAPISchemaGenCommandVisitor(QAPISchemaModularCVisitor): void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds); ''', - c_prefix=c_name(self._prefix, protect=False))) + c_prefix=c_name(self._prefix, protect=False))) self._genc.preamble_add(mcgen(''' #include "qemu/osdep.h" #include "%(prefix)sqapi-commands.h" diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index d7a289eded..fecf466fa7 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -35,7 +35,6 @@ def check_name_is_str(name, info, source): def check_name_str(name, info, source, allow_optional=False, enum_member=False, permit_upper=False): - global valid_name membername = name if allow_optional and name.startswith('*'): @@ -249,7 +248,7 @@ def check_union(expr, info): def check_alternate(expr, info): members = expr['data'] - if len(members) == 0: + if not members: raise QAPISemError(info, "'data' must not be empty") for (key, value) in members.items(): source = "'data' member '%s'" % key diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py index e17354392b..33690bfa3b 100644 --- a/scripts/qapi/gen.py +++ b/scripts/qapi/gen.py @@ -45,10 +45,10 @@ class QAPIGen: def write(self, output_dir): pathname = os.path.join(output_dir, self.fname) - dir = os.path.dirname(pathname) - if dir: + odir = os.path.dirname(pathname) + if odir: try: - os.makedirs(dir) + os.makedirs(odir) except os.error as e: if e.errno != errno.EEXIST: raise @@ -261,6 +261,9 @@ class QAPISchemaModularCVisitor(QAPISchemaVisitor): genc.write(output_dir) genh.write(output_dir) + def _begin_system_module(self, name): + pass + def _begin_user_module(self, name): pass diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py index 0cc655fd9f..b5537eddc0 100644 --- a/scripts/qapi/introspect.py +++ b/scripts/qapi/introspect.py @@ -10,8 +10,6 @@ This work is licensed under the terms of the GNU GPL, version 2. See the COPYING file in the top-level directory. """ -import string - from qapi.common import * from qapi.gen import QAPISchemaMonolithicCVisitor from qapi.schema import (QAPISchemaArrayType, QAPISchemaBuiltinType, diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index 340f7c4633..abadacbb0e 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -282,8 +282,7 @@ class QAPISchemaParser: doc.end_comment() self.accept() return doc - else: - doc.append(self.val) + doc.append(self.val) self.accept(False) raise QAPIParseError(self, "documentation comment must end with '##'") @@ -492,7 +491,7 @@ class QAPIDoc: raise QAPIParseError(self._parser, "'%s' can't follow '%s' section" % (name, self.sections[0].name)) - elif self._is_section_tag(name): + if self._is_section_tag(name): line = line[len(name)+1:] self._start_section(name[:-1]) @@ -556,7 +555,6 @@ class QAPIDoc: raise QAPISemError(feature.info, "feature '%s' lacks documentation" % feature.name) - self.features[feature.name] = QAPIDoc.ArgSection(feature.name) self.features[feature.name].connect(feature) def check_expr(self, expr): diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index e132442c04..cfbb9758c4 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -19,7 +19,7 @@ import re from collections import OrderedDict from qapi.common import c_name, pointer_suffix -from qapi.error import QAPIError, QAPIParseError, QAPISemError +from qapi.error import QAPIError, QAPISemError from qapi.expr import check_exprs from qapi.parser import QAPISchemaParser @@ -96,14 +96,14 @@ class QAPISchemaVisitor: def visit_end(self): pass - def visit_module(self, fname): + def visit_module(self, name): pass def visit_needed(self, entity): # Default to visiting everything return True - def visit_include(self, fname, info): + def visit_include(self, name, info): pass def visit_builtin_type(self, name, info, json_type): @@ -576,7 +576,7 @@ class QAPISchemaObjectTypeVariants: assert self.tag_member.ifcond == [] if self._tag_name: # flat union # branches that are not explicitly covered get an empty type - cases = set([v.name for v in self.variants]) + cases = {v.name for v in self.variants} for m in self.tag_member.type.members: if m.name not in cases: v = QAPISchemaObjectTypeVariant(m.name, self.info, @@ -1098,7 +1098,6 @@ class QAPISchema: def visit(self, visitor): visitor.visit_begin(self) - module = None for mod in self._module_dict.values(): mod.visit(visitor) visitor.visit_end()
Signed-off-by: Markus Armbruster <armbru@redhat.com> --- scripts/qapi/commands.py | 2 +- scripts/qapi/expr.py | 3 +-- scripts/qapi/gen.py | 9 ++++++--- scripts/qapi/introspect.py | 2 -- scripts/qapi/parser.py | 6 ++---- scripts/qapi/schema.py | 9 ++++----- 6 files changed, 14 insertions(+), 17 deletions(-)