[libvirt] [PATCH 00/22] Python tweaks

These patches improve the code style of python code by applying some PEP8 recommendations and simplifying some functions. Radostin Stoyanov (22): apibuild: Use isinctance for type checking apibuild: Split imports on separate lines apibuild: Remove whitespace before ',' and ':' python: Add whitespace around = and % operators esx_vi_generator: Simplify generate_helper_source esx_vi_generator: Simplify generate_helper_header esx_vi_generator: Simplify get_occurrence_comment esx_vi_generator: Simplify alignment function apibuild: Simplify conditional statements python: Remove space around = in keyword args WmiClass: Don't share "versions" between instances apibuild: Simplify uniq function apibuild: Avoid double sorting of ids python3: cpu-reformat: Use the print() function apibuild: Drop backslash between brackets apibuild: Fix indentation not multiple of 4 apibuild: Simplify strip_lead_star() apibuild: Simplify parseTypeComment() apibuild: Simplify type checking of literals apibuild: Use list comprehension insteand of map apibuild: Simplify merging of preproc tokens apibuild: Simplify parsing string tokens docs/apibuild.py | 643 +++++++++++++++++-------------------- docs/index.py | 54 ++-- src/esx/esx_vi_generator.py | 179 +++++------ src/hyperv/hyperv_wmi_generator.py | 36 ++- tests/cputestdata/cpu-cpuid.py | 6 +- tests/cputestdata/cpu-reformat.py | 6 +- 6 files changed, 424 insertions(+), 500 deletions(-) -- 2.14.3

The isinstance() function [1] returns true if an object argument is an instance of a classinfo argument or of a direct, indirect subclass thereof. 1: https://docs.python.org/3/library/functions.html#isinstance Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- docs/apibuild.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/docs/apibuild.py b/docs/apibuild.py index 51abf8383..7faa083b2 100755 --- a/docs/apibuild.py +++ b/docs/apibuild.py @@ -742,7 +742,7 @@ class CParser: return line def cleanupComment(self): - if type(self.comment) != type(""): + if not isinstance(self.comment, str): return # remove the leading * on multi-line comments lines = self.comment.splitlines(True) @@ -2223,9 +2223,8 @@ class docBuilder: output.write(" <struct name='%s' file='%s' type='%s'" % ( name, self.modulename_file(id.header), id.info)) name = id.info[7:] - if name in self.idx.structs and ( \ - type(self.idx.structs[name].info) == type(()) or - type(self.idx.structs[name].info) == type([])): + if (name in self.idx.structs and + isinstance(self.idx.structs[name].info, (list, tuple))): output.write(">\n") try: for field in self.idx.structs[name].info: -- 2.14.3

s/isinctance/isinstance/ in the $subject On Sat, Mar 17, 2018 at 02:23:19PM +0000, Radostin Stoyanov wrote:
The isinstance() function [1] returns true if an object argument is an instance of a classinfo argument or of a direct, indirect subclass thereof.
1: https://docs.python.org/3/library/functions.html#isinstance
Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- docs/apibuild.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On Sat, 2018-03-17 at 14:23 +0000, Radostin Stoyanov wrote:
The isinstance() function [1] returns true if an object argument is an instance of a classinfo argument or of a direct, indirect subclass thereof.
1: https://docs.python.org/3/library/functions.html#isinstance
Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- docs/apibuild.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
s/isinctance/isinstance/ in the subject. -- Andrea Bolognani / Red Hat / Virtualization

PEP8 recommends imports to be on separate lines. [1] 1: https://www.python.org/dev/peps/pep-0008/#imports Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- docs/apibuild.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/apibuild.py b/docs/apibuild.py index 7faa083b2..190d5d93b 100755 --- a/docs/apibuild.py +++ b/docs/apibuild.py @@ -10,7 +10,8 @@ from __future__ import print_function -import os, sys +import os +import sys import glob import re -- 2.14.3

On Sat, Mar 17, 2018 at 02:23:20PM +0000, Radostin Stoyanov wrote:
PEP8 recommends imports to be on separate lines. [1]
1: https://www.python.org/dev/peps/pep-0008/#imports
Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- docs/apibuild.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

PEP8 recommends removing whitespace immediately before a comma, semicolon, or colon [1]. In addition remove multiple spaces after keyword (PEP8 - E271). 1: https://www.python.org/dev/peps/pep-0008/#whitespace-in-expressions-and-stat... Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- docs/apibuild.py | 102 +++++++++++++++++++------------------ docs/index.py | 22 ++++---- src/esx/esx_vi_generator.py | 102 +++++++++++++++++-------------------- src/hyperv/hyperv_wmi_generator.py | 32 ++++++------ 4 files changed, 127 insertions(+), 131 deletions(-) diff --git a/docs/apibuild.py b/docs/apibuild.py index 190d5d93b..582c661e7 100755 --- a/docs/apibuild.py +++ b/docs/apibuild.py @@ -590,7 +590,7 @@ class CLexer: # line[i] == '>' or line[i] == '<' or line[i] == '=' or \ # line[i] == '/' or line[i] == '%' or line[i] == '&' or \ # line[i] == '!' or line[i] == '|' or line[i] == '.': - if line[i] == '.' and i + 2 < l and \ + if line[i] == '.' and i + 2 < l and \ line[i+1] == '.' and line[i+2] == '.': self.tokens.append(('name', '...')) i = i + 3 @@ -1735,7 +1735,7 @@ class CParser: while token is not None and token[0] == "op" and token[1] == '*': self.type = self.type + token[1] token = self.token() - if token is None or token[0] != "name" : + if token is None or token[0] != "name": self.error("parsing function type, name expected", token) return token self.type = self.type + token[1] @@ -1825,41 +1825,42 @@ class CParser: # this dict contains the functions that are allowed to use [unsigned] # long for legacy reasons in their signature and return type. this list is # fixed. new procedures and public APIs have to use [unsigned] long long - long_legacy_functions = \ - { "virGetVersion" : (False, ("libVer", "typeVer")), - "virConnectGetLibVersion" : (False, ("libVer")), - "virConnectGetVersion" : (False, ("hvVer")), - "virDomainGetMaxMemory" : (True, ()), - "virDomainMigrate" : (False, ("flags", "bandwidth")), - "virDomainMigrate2" : (False, ("flags", "bandwidth")), - "virDomainMigrateBegin3" : (False, ("flags", "bandwidth")), - "virDomainMigrateConfirm3" : (False, ("flags", "bandwidth")), - "virDomainMigrateDirect" : (False, ("flags", "bandwidth")), - "virDomainMigrateFinish" : (False, ("flags")), - "virDomainMigrateFinish2" : (False, ("flags")), - "virDomainMigrateFinish3" : (False, ("flags")), - "virDomainMigratePeer2Peer" : (False, ("flags", "bandwidth")), - "virDomainMigratePerform" : (False, ("flags", "bandwidth")), - "virDomainMigratePerform3" : (False, ("flags", "bandwidth")), - "virDomainMigratePrepare" : (False, ("flags", "bandwidth")), - "virDomainMigratePrepare2" : (False, ("flags", "bandwidth")), - "virDomainMigratePrepare3" : (False, ("flags", "bandwidth")), - "virDomainMigratePrepareTunnel" : (False, ("flags", "bandwidth")), - "virDomainMigratePrepareTunnel3" : (False, ("flags", "bandwidth")), - "virDomainMigrateToURI" : (False, ("flags", "bandwidth")), - "virDomainMigrateToURI2" : (False, ("flags", "bandwidth")), - "virDomainMigrateVersion1" : (False, ("flags", "bandwidth")), - "virDomainMigrateVersion2" : (False, ("flags", "bandwidth")), - "virDomainMigrateVersion3" : (False, ("flags", "bandwidth")), - "virDomainMigrateSetMaxSpeed" : (False, ("bandwidth")), - "virDomainSetMaxMemory" : (False, ("memory")), - "virDomainSetMemory" : (False, ("memory")), - "virDomainSetMemoryFlags" : (False, ("memory")), - "virDomainBlockCommit" : (False, ("bandwidth")), - "virDomainBlockJobSetSpeed" : (False, ("bandwidth")), - "virDomainBlockPull" : (False, ("bandwidth")), - "virDomainBlockRebase" : (False, ("bandwidth")), - "virDomainMigrateGetMaxSpeed" : (False, ("bandwidth")) } + long_legacy_functions = { + "virGetVersion": (False, ("libVer", "typeVer")), + "virConnectGetLibVersion": (False, ("libVer")), + "virConnectGetVersion": (False, ("hvVer")), + "virDomainGetMaxMemory": (True, ()), + "virDomainMigrate": (False, ("flags", "bandwidth")), + "virDomainMigrate2": (False, ("flags", "bandwidth")), + "virDomainMigrateBegin3": (False, ("flags", "bandwidth")), + "virDomainMigrateConfirm3": (False, ("flags", "bandwidth")), + "virDomainMigrateDirect": (False, ("flags", "bandwidth")), + "virDomainMigrateFinish": (False, ("flags")), + "virDomainMigrateFinish2": (False, ("flags")), + "virDomainMigrateFinish3": (False, ("flags")), + "virDomainMigratePeer2Peer": (False, ("flags", "bandwidth")), + "virDomainMigratePerform": (False, ("flags", "bandwidth")), + "virDomainMigratePerform3": (False, ("flags", "bandwidth")), + "virDomainMigratePrepare": (False, ("flags", "bandwidth")), + "virDomainMigratePrepare2": (False, ("flags", "bandwidth")), + "virDomainMigratePrepare3": (False, ("flags", "bandwidth")), + "virDomainMigratePrepareTunnel": (False, ("flags", "bandwidth")), + "virDomainMigratePrepareTunnel3": (False, ("flags", "bandwidth")), + "virDomainMigrateToURI": (False, ("flags", "bandwidth")), + "virDomainMigrateToURI2": (False, ("flags", "bandwidth")), + "virDomainMigrateVersion1": (False, ("flags", "bandwidth")), + "virDomainMigrateVersion2": (False, ("flags", "bandwidth")), + "virDomainMigrateVersion3": (False, ("flags", "bandwidth")), + "virDomainMigrateSetMaxSpeed": (False, ("bandwidth")), + "virDomainSetMaxMemory": (False, ("memory")), + "virDomainSetMemory": (False, ("memory")), + "virDomainSetMemoryFlags": (False, ("memory")), + "virDomainBlockCommit": (False, ("bandwidth")), + "virDomainBlockJobSetSpeed": (False, ("bandwidth")), + "virDomainBlockPull": (False, ("bandwidth")), + "virDomainBlockRebase": (False, ("bandwidth")), + "virDomainMigrateGetMaxSpeed": (False, ("bandwidth")) + } def checkLongLegacyFunction(self, name, return_type, signature): if "long" in return_type and "long long" not in return_type: @@ -1883,10 +1884,11 @@ class CParser: # this dict contains the structs that are allowed to use [unsigned] # long for legacy reasons. this list is fixed. new structs have to use # [unsigned] long long - long_legacy_struct_fields = \ - { "_virDomainInfo" : ("maxMem", "memory"), - "_virNodeInfo" : ("memory"), - "_virDomainBlockJobInfo" : ("bandwidth") } + long_legacy_struct_fields = { + "_virDomainInfo": ("maxMem", "memory"), + "_virNodeInfo": ("memory"), + "_virDomainBlockJobInfo": ("bandwidth") + } def checkLongLegacyStruct(self, name, fields): for field in fields: @@ -1934,7 +1936,7 @@ class CParser: elif token[1] == 'static': static = 1 token = self.token() - if token is None or token[0] != 'name': + if token is None or token[0] != 'name': return token if token[1] == 'typedef': @@ -2205,7 +2207,7 @@ class docBuilder: output.write(" </macro>\n") def serialize_union(self, output, field, desc): - output.write(" <field name='%s' type='union' info='%s'>\n" % (field[1] , desc)) + output.write(" <field name='%s' type='union' info='%s'>\n" % (field[1], desc)) output.write(" <union>\n") for f in field[3]: desc = f[2] @@ -2213,7 +2215,7 @@ class docBuilder: desc = '' else: desc = escape(desc) - output.write(" <field name='%s' type='%s' info='%s'/>\n" % (f[1] , f[0], desc)) + output.write(" <field name='%s' type='%s' info='%s'/>\n" % (f[1], f[0], desc)) output.write(" </union>\n") output.write(" </field>\n") @@ -2238,13 +2240,13 @@ class docBuilder: if field[0] == "union": self.serialize_union(output, field, desc) else: - output.write(" <field name='%s' type='%s' info='%s'/>\n" % (field[1] , field[0], desc)) + output.write(" <field name='%s' type='%s' info='%s'/>\n" % (field[1], field[0], desc)) except: self.warning("Failed to serialize struct %s" % (name)) output.write(" </struct>\n") else: output.write("/>\n") - else : + else: output.write(" <typedef name='%s' file='%s' type='%s'" % ( name, self.modulename_file(id.header), id.info)) try: @@ -2401,7 +2403,7 @@ class docBuilder: typ = sorted(funcs.keys()) for type in typ: if type == '' or type == 'void' or type == "int" or \ - type == "char *" or type == "const char *" : + type == "char *" or type == "const char *": continue output.write(" <type name='%s'>\n" % (type)) ids = funcs[type] @@ -2430,7 +2432,7 @@ class docBuilder: typ = sorted(funcs.keys()) for type in typ: if type == '' or type == 'void' or type == "int" or \ - type == "char *" or type == "const char *" : + type == "char *" or type == "const char *": continue output.write(" <type name='%s'>\n" % (type)) ids = sorted(funcs[type]) @@ -2589,7 +2591,7 @@ class app: builddir = os.path.abspath((os.environ["builddir"])) if srcdir == builddir: builddir = None - if glob.glob(srcdir + "/../src/libvirt.c") != [] : + if glob.glob(srcdir + "/../src/libvirt.c") != []: if not quiet: print("Rebuilding API description for %s" % name) dirs = [srcdir + "/../src", @@ -2599,7 +2601,7 @@ class app: not os.path.exists(srcdir + "/../include/libvirt/libvirt-common.h")): dirs.append(builddir + "/../include/libvirt") builder = docBuilder(name, srcdir, dirs, []) - elif glob.glob("src/libvirt.c") != [] : + elif glob.glob("src/libvirt.c") != []: if not quiet: print("Rebuilding API description for %s" % name) builder = docBuilder(name, srcdir, diff --git a/docs/index.py b/docs/index.py index bedec8ae0..e2f9185c3 100755 --- a/docs/index.py +++ b/docs/index.py @@ -59,21 +59,21 @@ libxml2.registerErrorHandler(callback, None) # to create them # TABLES={ - "symbols" : """CREATE TABLE symbols ( + "symbols": """CREATE TABLE symbols ( name varchar(255) BINARY NOT NULL, module varchar(255) BINARY NOT NULL, type varchar(25) NOT NULL, descr varchar(255), UNIQUE KEY name (name), KEY module (module))""", - "words" : """CREATE TABLE words ( + "words": """CREATE TABLE words ( name varchar(50) BINARY NOT NULL, symbol varchar(255) BINARY NOT NULL, relevance int, KEY name (name), KEY symbol (symbol), UNIQUE KEY ID (name, symbol))""", - "wordsHTML" : """CREATE TABLE wordsHTML ( + "wordsHTML": """CREATE TABLE wordsHTML ( name varchar(50) BINARY NOT NULL, resource varchar(255) BINARY NOT NULL, section varchar(255), @@ -82,30 +82,30 @@ TABLES={ KEY name (name), KEY resource (resource), UNIQUE KEY ref (name, resource))""", - "wordsArchive" : """CREATE TABLE wordsArchive ( + "wordsArchive": """CREATE TABLE wordsArchive ( name varchar(50) BINARY NOT NULL, ID int(11) NOT NULL, relevance int, KEY name (name), UNIQUE KEY ref (name, ID))""", - "pages" : """CREATE TABLE pages ( + "pages": """CREATE TABLE pages ( resource varchar(255) BINARY NOT NULL, title varchar(255) BINARY NOT NULL, UNIQUE KEY name (resource))""", - "archives" : """CREATE TABLE archives ( + "archives": """CREATE TABLE archives ( ID int(11) NOT NULL auto_increment, resource varchar(255) BINARY NOT NULL, title varchar(255) BINARY NOT NULL, UNIQUE KEY id (ID,resource(255)), INDEX (ID), INDEX (resource))""", - "Queries" : """CREATE TABLE Queries ( + "Queries": """CREATE TABLE Queries ( ID int(11) NOT NULL auto_increment, Value varchar(50) NOT NULL, Count int(11) NOT NULL, UNIQUE KEY id (ID,Value(35)), INDEX (ID))""", - "AllQueries" : """CREATE TABLE AllQueries ( + "AllQueries": """CREATE TABLE AllQueries ( ID int(11) NOT NULL auto_increment, Value varchar(50) NOT NULL, Count int(11) NOT NULL, @@ -171,7 +171,7 @@ def checkTables(db, verbose = 1): if verbose: print "Table %s contains %d records" % (table, row[0]) except: - print "Troubles with table %s : repairing" % (table) + print "Troubles with table %s: repairing" % (table) ret = c.execute("repair table %s" % table) print "repairing returned %d" % (ret) ret = c.execute("SELECT count(*) from %s" % table) @@ -1041,7 +1041,7 @@ def analyzeHTMLPages(): doc = libxml2.htmlParseFile(html, None) try: res = analyzeHTML(doc, html) - print "Parsed %s : %d paragraphs" % (html, res) + print "Parsed %s: %d paragraphs" % (html, res) ret = ret + 1 except: print "could not parse %s" % (html) @@ -1230,7 +1230,7 @@ def main(): elif args[i] == '--archive-year': i = i + 1 year = args[i] - months = ["January" , "February", "March", "April", "May", + months = ["January", "February", "March", "April", "May", "June", "July", "August", "September", "October", "November", "December"] for month in months: diff --git a/src/esx/esx_vi_generator.py b/src/esx/esx_vi_generator.py index 6ce017d79..8fbc8bef1 100755 --- a/src/esx/esx_vi_generator.py +++ b/src/esx/esx_vi_generator.py @@ -1342,61 +1342,53 @@ predefined_objects = ["AnyType", "MethodFault", "ManagedObjectReference"] -additional_enum_features = { "ManagedEntityStatus" : Enum.FEATURE__ANY_TYPE, - "TaskInfoState" : Enum.FEATURE__ANY_TYPE, - "VirtualMachinePowerState" : Enum.FEATURE__ANY_TYPE } - -additional_object_features = { "AutoStartDefaults" : Object.FEATURE__ANY_TYPE, - "AutoStartPowerInfo" : Object.FEATURE__ANY_TYPE, - "DatastoreHostMount" : Object.FEATURE__DEEP_COPY | - Object.FEATURE__LIST | - Object.FEATURE__ANY_TYPE, - "DatastoreInfo" : Object.FEATURE__ANY_TYPE | - Object.FEATURE__DYNAMIC_CAST, - "HostConfigManager" : Object.FEATURE__ANY_TYPE, - "HostCpuIdInfo" : Object.FEATURE__LIST | - Object.FEATURE__ANY_TYPE, - "HostDatastoreBrowserSearchResults" : Object.FEATURE__LIST | - Object.FEATURE__ANY_TYPE, - "HostHostBusAdapter" : Object.FEATURE__LIST | - Object.FEATURE__ANY_TYPE, - "HostInternetScsiHba" : Object.FEATURE__DYNAMIC_CAST | - Object.FEATURE__DEEP_COPY, - "HostInternetScsiTargetTransport" : Object.FEATURE__DYNAMIC_CAST, - "HostScsiDisk" : Object.FEATURE__LIST | - Object.FEATURE__ANY_TYPE | - Object.FEATURE__DYNAMIC_CAST, - "HostScsiTopologyInterface" : Object.FEATURE__LIST | - Object.FEATURE__ANY_TYPE, - "HostScsiTopologyLun" : Object.FEATURE__ANY_TYPE | - Object.FEATURE__LIST | - Object.FEATURE__DEEP_COPY, - "HostScsiTopologyTarget" : Object.FEATURE__ANY_TYPE | - Object.FEATURE__LIST, - "HostPortGroup" : Object.FEATURE__LIST | - Object.FEATURE__ANY_TYPE, - "HostVirtualSwitch" : Object.FEATURE__DEEP_COPY | - Object.FEATURE__LIST | - Object.FEATURE__ANY_TYPE, - "ManagedObjectReference" : Object.FEATURE__ANY_TYPE, - "ObjectContent" : Object.FEATURE__DEEP_COPY, - "PhysicalNic" : Object.FEATURE__DEEP_COPY | - Object.FEATURE__LIST | - Object.FEATURE__ANY_TYPE, - "ResourcePoolResourceUsage" : Object.FEATURE__ANY_TYPE, - "ScsiLun" : Object.FEATURE__LIST | - Object.FEATURE__ANY_TYPE | - Object.FEATURE__DEEP_COPY, - "ScsiLunDurableName" : Object.FEATURE__LIST, - "ServiceContent" : Object.FEATURE__DESERIALIZE, - "SharesInfo" : Object.FEATURE__ANY_TYPE, - "TaskInfo" : Object.FEATURE__LIST | - Object.FEATURE__ANY_TYPE, - "UserSession" : Object.FEATURE__ANY_TYPE, - "VirtualMachineQuestionInfo" : Object.FEATURE__ANY_TYPE, - "VirtualMachineSnapshotTree" : Object.FEATURE__DEEP_COPY | - Object.FEATURE__ANY_TYPE, - "VmEventArgument" : Object.FEATURE__DESERIALIZE } +additional_enum_features = { + "ManagedEntityStatus": Enum.FEATURE__ANY_TYPE, + "TaskInfoState": Enum.FEATURE__ANY_TYPE, + "VirtualMachinePowerState": Enum.FEATURE__ANY_TYPE +} + +additional_object_features = { + "AutoStartDefaults": Object.FEATURE__ANY_TYPE, + "AutoStartPowerInfo": Object.FEATURE__ANY_TYPE, + "DatastoreHostMount": (Object.FEATURE__DEEP_COPY | Object.FEATURE__LIST | + Object.FEATURE__ANY_TYPE), + "DatastoreInfo": Object.FEATURE__ANY_TYPE | Object.FEATURE__DYNAMIC_CAST, + "HostConfigManager": Object.FEATURE__ANY_TYPE, + "HostCpuIdInfo": Object.FEATURE__LIST | Object.FEATURE__ANY_TYPE, + "HostDatastoreBrowserSearchResults": (Object.FEATURE__LIST | + Object.FEATURE__ANY_TYPE), + "HostHostBusAdapter": Object.FEATURE__LIST | Object.FEATURE__ANY_TYPE, + "HostInternetScsiHba": (Object.FEATURE__DYNAMIC_CAST | + Object.FEATURE__DEEP_COPY), + "HostInternetScsiTargetTransport": Object.FEATURE__DYNAMIC_CAST, + "HostScsiDisk": (Object.FEATURE__LIST | Object.FEATURE__ANY_TYPE | + Object.FEATURE__DYNAMIC_CAST), + "HostScsiTopologyInterface": (Object.FEATURE__LIST | + Object.FEATURE__ANY_TYPE), + "HostScsiTopologyLun": (Object.FEATURE__ANY_TYPE | Object.FEATURE__LIST | + Object.FEATURE__DEEP_COPY), + "HostScsiTopologyTarget": Object.FEATURE__ANY_TYPE | Object.FEATURE__LIST, + "HostPortGroup": Object.FEATURE__LIST | Object.FEATURE__ANY_TYPE, + "HostVirtualSwitch": (Object.FEATURE__DEEP_COPY | Object.FEATURE__LIST | + Object.FEATURE__ANY_TYPE), + "ManagedObjectReference": Object.FEATURE__ANY_TYPE, + "ObjectContent": Object.FEATURE__DEEP_COPY, + "PhysicalNic": (Object.FEATURE__DEEP_COPY | Object.FEATURE__LIST | + Object.FEATURE__ANY_TYPE), + "ResourcePoolResourceUsage": Object.FEATURE__ANY_TYPE, + "ScsiLun": (Object.FEATURE__LIST | Object.FEATURE__ANY_TYPE | + Object.FEATURE__DEEP_COPY), + "ScsiLunDurableName": Object.FEATURE__LIST, + "ServiceContent": Object.FEATURE__DESERIALIZE, + "SharesInfo": Object.FEATURE__ANY_TYPE, + "TaskInfo": Object.FEATURE__LIST | Object.FEATURE__ANY_TYPE, + "UserSession": Object.FEATURE__ANY_TYPE, + "VirtualMachineQuestionInfo": Object.FEATURE__ANY_TYPE, + "VirtualMachineSnapshotTree": (Object.FEATURE__DEEP_COPY | + Object.FEATURE__ANY_TYPE), + "VmEventArgument": Object.FEATURE__DESERIALIZE +} removed_object_features = {} diff --git a/src/hyperv/hyperv_wmi_generator.py b/src/hyperv/hyperv_wmi_generator.py index d54810211..f98a77562 100755 --- a/src/hyperv/hyperv_wmi_generator.py +++ b/src/hyperv/hyperv_wmi_generator.py @@ -342,21 +342,23 @@ class WmiClassVersion: class Property: - typemap = {"boolean" : "BOOL", - "string" : "STR", - "datetime" : "STR", - "int8" : "INT8", - "sint8" : "INT8", - "int16" : "INT16", - "sint16" : "INT16", - "int32" : "INT32", - "sint32" : "INT32", - "int64" : "INT64", - "sint64" : "INT64", - "uint8" : "UINT8", - "uint16" : "UINT16", - "uint32" : "UINT32", - "uint64" : "UINT64"} + typemap = { + "boolean": "BOOL", + "string": "STR", + "datetime": "STR", + "int8": "INT8", + "sint8": "INT8", + "int16": "INT16", + "sint16": "INT16", + "int32": "INT32", + "sint32": "INT32", + "int64": "INT64", + "sint64": "INT64", + "uint8": "UINT8", + "uint16": "UINT16", + "uint32": "UINT32", + "uint64": "UINT64" + } def __init__(self, type, name, is_array): -- 2.14.3

On Sat, Mar 17, 2018 at 02:23:21PM +0000, Radostin Stoyanov wrote:
PEP8 recommends removing whitespace immediately before a comma, semicolon, or colon [1]. In addition remove multiple spaces after keyword (PEP8 - E271).
1: https://www.python.org/dev/peps/pep-0008/#whitespace-in-expressions-and-stat...
Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- docs/apibuild.py | 102 +++++++++++++++++++------------------ docs/index.py | 22 ++++---- src/esx/esx_vi_generator.py | 102 +++++++++++++++++-------------------- src/hyperv/hyperv_wmi_generator.py | 32 ++++++------ 4 files changed, 127 insertions(+), 131 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- docs/apibuild.py | 18 +++++++++--------- docs/index.py | 6 +++--- tests/cputestdata/cpu-cpuid.py | 2 +- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/docs/apibuild.py b/docs/apibuild.py index 582c661e7..644d96f69 100755 --- a/docs/apibuild.py +++ b/docs/apibuild.py @@ -15,10 +15,10 @@ import sys import glob import re -quiet=True -warnings=0 -debug=False -debugsym=None +quiet = True +warnings = 0 +debug = False +debugsym = None # # C parser analysis code @@ -132,7 +132,7 @@ def escape(raw): def uniq(items): d = {} for item in items: - d[item]=1 + d[item] = 1 k = sorted(d.keys()) return k @@ -1408,7 +1408,7 @@ class CParser: value = "%d" % (int(value) + 1) except: self.warning("Failed to compute value of enum %s" % (name)) - value="" + value = "" if token[0] == "sep" and token[1] == ",": if commentsBeforeVal: self.cleanupComment() @@ -2286,7 +2286,7 @@ class docBuilder: if apstr != "": apstr = apstr + " && " apstr = apstr + cond - output.write(" <cond>%s</cond>\n"% (apstr)) + output.write(" <cond>%s</cond>\n" % (apstr)) try: (ret, params, desc) = id.info output.write(" <info><![CDATA[%s]]></info>\n" % (desc)) @@ -2479,7 +2479,7 @@ class docBuilder: output.write(" </letter>\n") output.write(" </chunk>\n") count = 0 - chunks.append(["chunk%s" % (chunk -1), first_letter, letter]) + chunks.append(["chunk%s" % (chunk - 1), first_letter, letter]) output.write(" <chunk name='chunk%s'>\n" % (chunk)) first_letter = id[0] chunk = chunk + 1 @@ -2502,7 +2502,7 @@ class docBuilder: output.write(" </letter>\n") output.write(" </chunk>\n") if count != 0: - chunks.append(["chunk%s" % (chunk -1), first_letter, letter]) + chunks.append(["chunk%s" % (chunk - 1), first_letter, letter]) output.write(" <chunks>\n") for ch in chunks: output.write(" <chunk name='%s' start='%s' end='%s'/>\n" % ( diff --git a/docs/index.py b/docs/index.py index e2f9185c3..5224324b1 100755 --- a/docs/index.py +++ b/docs/index.py @@ -58,7 +58,7 @@ libxml2.registerErrorHandler(callback, None) # The dictionary of tables required and the SQL command needed # to create them # -TABLES={ +TABLES = { "symbols": """CREATE TABLE symbols ( name varchar(255) BINARY NOT NULL, module varchar(255) BINARY NOT NULL, @@ -116,8 +116,8 @@ TABLES={ # # The XML API description file to parse # -API="libvirt-api.xml" -DB=None +API = "libvirt-api.xml" +DB = None ######################################################################### # # diff --git a/tests/cputestdata/cpu-cpuid.py b/tests/cputestdata/cpu-cpuid.py index b74c3ce93..43c92731a 100755 --- a/tests/cputestdata/cpu-cpuid.py +++ b/tests/cputestdata/cpu-cpuid.py @@ -325,7 +325,7 @@ def formatCpuid(cpuid, path, comment): line = (" <cpuid eax_in='0x%08x' ecx_in='0x%02x' " "eax='0x%08x' ebx='0x%08x' " "ecx='0x%08x' edx='0x%08x'/>\n") - f.write(line %( + f.write(line % ( in_eax, in_ecx, leaf["eax"], leaf["ebx"], leaf["ecx"], leaf["edx"])) f.write("</cpudata>\n") -- 2.14.3

On Sat, Mar 17, 2018 at 02:23:22PM +0000, Radostin Stoyanov wrote:
Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- docs/apibuild.py | 18 +++++++++--------- docs/index.py | 6 +++--- tests/cputestdata/cpu-cpuid.py | 2 +- 3 files changed, 13 insertions(+), 13 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

The generate_helper_source() function returns a formatted string. This could be achieved without the use of a local variable "source" and string concatenation. Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- src/esx/esx_vi_generator.py | 29 ++++++++++------------------- 1 file changed, 10 insertions(+), 19 deletions(-) diff --git a/src/esx/esx_vi_generator.py b/src/esx/esx_vi_generator.py index 8fbc8bef1..19c225384 100755 --- a/src/esx/esx_vi_generator.py +++ b/src/esx/esx_vi_generator.py @@ -1092,26 +1092,17 @@ class ManagedObject(GenericObject): def generate_helper_source(self): - source = "" - # lookup - source += "/* esxVI_Lookup%s */\n" % self.name - source += "ESX_VI__TEMPLATE__LOOKUP(%s,\n" % self.name - source += "{\n" - - source += self.generate_lookup_code1() - - source += "},\n" - source += "{\n" - - source += self.generate_lookup_code2() - - source += "})\n\n" - - source += "\n\n" - - return source - + return ( + "/* esxVI_Lookup{name} */\n" + "ESX_VI__TEMPLATE__LOOKUP({name},\n" + "{{\n{lookup_code1}}},\n" + "{{\n{lookup_code2}}})\n\n\n\n" + ).format( + name=self.name, + lookup_code1=self.generate_lookup_code1(), + lookup_code2=self.generate_lookup_code2() + ) class Enum(Type): -- 2.14.3

On Sat, Mar 17, 2018 at 02:23:23PM +0000, Radostin Stoyanov wrote:
The generate_helper_source() function returns a formatted string. This could be achieved without the use of a local variable "source" and string concatenation.
Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- src/esx/esx_vi_generator.py | 29 ++++++++++------------------- 1 file changed, 10 insertions(+), 19 deletions(-)
diff --git a/src/esx/esx_vi_generator.py b/src/esx/esx_vi_generator.py index 8fbc8bef1..19c225384 100755 --- a/src/esx/esx_vi_generator.py +++ b/src/esx/esx_vi_generator.py @@ -1092,26 +1092,17 @@ class ManagedObject(GenericObject):
def generate_helper_source(self): - source = "" - # lookup - source += "/* esxVI_Lookup%s */\n" % self.name - source += "ESX_VI__TEMPLATE__LOOKUP(%s,\n" % self.name - source += "{\n" - - source += self.generate_lookup_code1() - - source += "},\n" - source += "{\n" - - source += self.generate_lookup_code2() - - source += "})\n\n" - - source += "\n\n" - - return source - + return ( + "/* esxVI_Lookup{name} */\n" + "ESX_VI__TEMPLATE__LOOKUP({name},\n" + "{{\n{lookup_code1}}},\n" + "{{\n{lookup_code2}}})\n\n\n\n"
Most of our code uses the traditional printf style format variables, and I think that would be nicer here, because of the usage of literal "{" in places It woukd be clear to break strings everywhere there is a newline, to make it resemble the final generated code structure better.
+ ).format( + name=self.name, + lookup_code1=self.generate_lookup_code1(), + lookup_code2=self.generate_lookup_code2() + )
class Enum(Type): -- 2.14.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On Sat, Mar 17, 2018 at 02:23:23PM +0000, Radostin Stoyanov wrote:
The generate_helper_source() function returns a formatted string. This could be achieved without the use of a local variable "source" and string concatenation.
Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- src/esx/esx_vi_generator.py | 29 ++++++++++------------------- 1 file changed, 10 insertions(+), 19 deletions(-)
diff --git a/src/esx/esx_vi_generator.py b/src/esx/esx_vi_generator.py index 8fbc8bef1..19c225384 100755 --- a/src/esx/esx_vi_generator.py +++ b/src/esx/esx_vi_generator.py @@ -1092,26 +1092,17 @@ class ManagedObject(GenericObject):
def generate_helper_source(self): - source = "" - # lookup - source += "/* esxVI_Lookup%s */\n" % self.name - source += "ESX_VI__TEMPLATE__LOOKUP(%s,\n" % self.name - source += "{\n" - - source += self.generate_lookup_code1() - - source += "},\n" - source += "{\n" - - source += self.generate_lookup_code2() - - source += "})\n\n" - - source += "\n\n" - - return source - + return ( + "/* esxVI_Lookup{name} */\n" + "ESX_VI__TEMPLATE__LOOKUP({name},\n" + "{{\n{lookup_code1}}},\n" + "{{\n{lookup_code2}}})\n\n\n\n" Most of our code uses the traditional printf style format variables, and I think that would be nicer here, because of the usage of literal "{" in places
It woukd be clear to break strings everywhere there is a newline, to make it resemble the final generated code structure better. Thanks, I will resend this and the next patch with preserving the
On 19/03/18 10:42, Daniel P. Berrangé wrote: traditional printf style format, and break strings everywhere there is a newline.
+ ).format( + name=self.name, + lookup_code1=self.generate_lookup_code1(), + lookup_code2=self.generate_lookup_code2() + )
class Enum(Type): -- 2.14.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list Regards, Daniel

The function generate_helper_header() only returns a formatted string. This could be achieved without performing string concatenation. Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- src/esx/esx_vi_generator.py | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/src/esx/esx_vi_generator.py b/src/esx/esx_vi_generator.py index 19c225384..1641a2a1e 100755 --- a/src/esx/esx_vi_generator.py +++ b/src/esx/esx_vi_generator.py @@ -1021,20 +1021,15 @@ class ManagedObject(GenericObject): def generate_helper_header(self): - header = "" - # functions - header += ("int esxVI_Lookup%s(esxVI_Context *ctx, " - "const char *name, " - "esxVI_ManagedObjectReference *root, " - "esxVI_String *selectedPropertyNameList, " - "esxVI_%s **item, " - "esxVI_Occurrence occurrence);\n") \ - % (self.name, self.name) - - header += "\n" - - return header + return ( + "int esxVI_Lookup{name}(esxVI_Context *ctx, " + "const char *name, " + "esxVI_ManagedObjectReference *root, " + "esxVI_String *selectedPropertyNameList, " + "esxVI_{name} **item, " + "esxVI_Occurrence occurrence);\n\n" + ).format(name=self.name) def generate_source(self): -- 2.14.3

On Sat, Mar 17, 2018 at 02:23:24PM +0000, Radostin Stoyanov wrote:
The function generate_helper_header() only returns a formatted string. This could be achieved without performing string concatenation.
Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- src/esx/esx_vi_generator.py | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-)
diff --git a/src/esx/esx_vi_generator.py b/src/esx/esx_vi_generator.py index 19c225384..1641a2a1e 100755 --- a/src/esx/esx_vi_generator.py +++ b/src/esx/esx_vi_generator.py @@ -1021,20 +1021,15 @@ class ManagedObject(GenericObject):
def generate_helper_header(self): - header = "" - # functions - header += ("int esxVI_Lookup%s(esxVI_Context *ctx, " - "const char *name, " - "esxVI_ManagedObjectReference *root, " - "esxVI_String *selectedPropertyNameList, " - "esxVI_%s **item, " - "esxVI_Occurrence occurrence);\n") \ - % (self.name, self.name) - - header += "\n" - - return header + return ( + "int esxVI_Lookup{name}(esxVI_Context *ctx, " + "const char *name, " + "esxVI_ManagedObjectReference *root, " + "esxVI_String *selectedPropertyNameList, " + "esxVI_{name} **item, " + "esxVI_Occurrence occurrence);\n\n" + ).format(name=self.name)
You're mixing two different changes in one patch here. First you're getting rid of the string concatenation, but second you are changing the printf format syntax used. Each change should be a distinct patch, but even better is to not change the printf format at all. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Reduce the number of if-statements and use a single return. Utilise a dictionary to map between occurrences and values. Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- src/esx/esx_vi_generator.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/esx/esx_vi_generator.py b/src/esx/esx_vi_generator.py index 1641a2a1e..e4890c61b 100755 --- a/src/esx/esx_vi_generator.py +++ b/src/esx/esx_vi_generator.py @@ -75,16 +75,16 @@ class Member: def get_occurrence_comment(self): - if self.occurrence == OCCURRENCE__REQUIRED_ITEM: - return "/* required */" - elif self.occurrence == OCCURRENCE__REQUIRED_LIST: - return "/* required, list */" - elif self.occurrence == OCCURRENCE__OPTIONAL_ITEM: - return "/* optional */" - elif self.occurrence == OCCURRENCE__OPTIONAL_LIST: - return "/* optional, list */" - - raise ValueError("unknown occurrence value '%s'" % self.occurrence) + occurrence_map = { + OCCURRENCE__REQUIRED_ITEM: "/* required */", + OCCURRENCE__REQUIRED_LIST: "/* required, list */", + OCCURRENCE__OPTIONAL_ITEM: "/* optional */", + OCCURRENCE__OPTIONAL_LIST: "/* optional, list */" + } + if self.occurrence not in occurrence_map: + raise ValueError("unknown occurrence value '%s'" % self.occurrence) + + return occurrence_map[self.occurrence] -- 2.14.3

On Sat, Mar 17, 2018 at 02:23:25PM +0000, Radostin Stoyanov wrote:
Reduce the number of if-statements and use a single return. Utilise a dictionary to map between occurrences and values.
Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- src/esx/esx_vi_generator.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On Sat, Mar 17, 2018 at 03:23 PM +0100, Radostin Stoyanov <rstoyanov1@gmail.com> wrote:
Reduce the number of if-statements and use a single return. Utilise a dictionary to map between occurrences and values.
Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- src/esx/esx_vi_generator.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/src/esx/esx_vi_generator.py b/src/esx/esx_vi_generator.py index 1641a2a1e..e4890c61b 100755 --- a/src/esx/esx_vi_generator.py +++ b/src/esx/esx_vi_generator.py @@ -75,16 +75,16 @@ class Member:
def get_occurrence_comment(self): - if self.occurrence == OCCURRENCE__REQUIRED_ITEM: - return "/* required */" - elif self.occurrence == OCCURRENCE__REQUIRED_LIST: - return "/* required, list */" - elif self.occurrence == OCCURRENCE__OPTIONAL_ITEM: - return "/* optional */" - elif self.occurrence == OCCURRENCE__OPTIONAL_LIST: - return "/* optional, list */" - - raise ValueError("unknown occurrence value '%s'" % self.occurrence) + occurrence_map = { + OCCURRENCE__REQUIRED_ITEM: "/* required */", + OCCURRENCE__REQUIRED_LIST: "/* required, list */", + OCCURRENCE__OPTIONAL_ITEM: "/* optional */", + OCCURRENCE__OPTIONAL_LIST: "/* optional, list */" + } + if self.occurrence not in occurrence_map: + raise ValueError("unknown occurrence value '%s'" % self.occurrence) + + return occurrence_map[self.occurrence]
What do you think about this? (instead of an explicit check) try: return occurrence_map[self.occurrence] except KeyError: raise ValueError("unknown occurrence value '%s'" % self.occurrence) In my opinion it’s more “pythonic”. Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294

On 19/03/18 11:40, Marc Hartmayer wrote:
On Sat, Mar 17, 2018 at 03:23 PM +0100, Radostin Stoyanov <rstoyanov1@gmail.com> wrote:
Reduce the number of if-statements and use a single return. Utilise a dictionary to map between occurrences and values.
Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- src/esx/esx_vi_generator.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/src/esx/esx_vi_generator.py b/src/esx/esx_vi_generator.py index 1641a2a1e..e4890c61b 100755 --- a/src/esx/esx_vi_generator.py +++ b/src/esx/esx_vi_generator.py @@ -75,16 +75,16 @@ class Member:
def get_occurrence_comment(self): - if self.occurrence == OCCURRENCE__REQUIRED_ITEM: - return "/* required */" - elif self.occurrence == OCCURRENCE__REQUIRED_LIST: - return "/* required, list */" - elif self.occurrence == OCCURRENCE__OPTIONAL_ITEM: - return "/* optional */" - elif self.occurrence == OCCURRENCE__OPTIONAL_LIST: - return "/* optional, list */" - - raise ValueError("unknown occurrence value '%s'" % self.occurrence) + occurrence_map = { + OCCURRENCE__REQUIRED_ITEM: "/* required */", + OCCURRENCE__REQUIRED_LIST: "/* required, list */", + OCCURRENCE__OPTIONAL_ITEM: "/* optional */", + OCCURRENCE__OPTIONAL_LIST: "/* optional, list */" + } + if self.occurrence not in occurrence_map: + raise ValueError("unknown occurrence value '%s'" % self.occurrence) + + return occurrence_map[self.occurrence] What do you think about this? (instead of an explicit check)
try: return occurrence_map[self.occurrence] except KeyError: raise ValueError("unknown occurrence value '%s'" % self.occurrence)
In my opinion it’s more “pythonic”. Thanks Marc, Yes, I agree with you. Using try/except here would be better! Beste Grüße / Kind regards Marc Hartmayer
IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294

Generate whitespace using the standard function ljust() that is available in both Py3 [1] and Py2 [2]. 1: https://docs.python.org/3/library/stdtypes.html?highlight=strip#str.ljust 2: https://docs.python.org/2.7/library/string.html#string.ljust Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- src/esx/esx_vi_generator.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/esx/esx_vi_generator.py b/src/esx/esx_vi_generator.py index e4890c61b..0dca44fed 100755 --- a/src/esx/esx_vi_generator.py +++ b/src/esx/esx_vi_generator.py @@ -49,10 +49,7 @@ separator = "/* " + ("* " * 37) + "*\n" def aligned(left, right, length=59): - while len(left) < length: - left += " " - - return left + right + return left.ljust(length, ' ') + right -- 2.14.3

On Sat, Mar 17, 2018 at 02:23:26PM +0000, Radostin Stoyanov wrote:
Generate whitespace using the standard function ljust() that is available in both Py3 [1] and Py2 [2].
1: https://docs.python.org/3/library/stdtypes.html?highlight=strip#str.ljust 2: https://docs.python.org/2.7/library/string.html#string.ljust
Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- src/esx/esx_vi_generator.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Improve readability by reducing the complexity and length of conditional statements. Example: The following condition: if (o >= 97 and o <= 122) or (o >= 65 and o <= 90) or (o >= 48 and o <= 57) or (" \t(){}:;,+-*/%&!|[]=><".find(line[i]) == -1): Will be True for every character that is not in string: " \t(){}:;,+-*/%&!|[]=><" Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- docs/apibuild.py | 33 ++++++++++----------------------- 1 file changed, 10 insertions(+), 23 deletions(-) diff --git a/docs/apibuild.py b/docs/apibuild.py index 644d96f69..1b9401226 100755 --- a/docs/apibuild.py +++ b/docs/apibuild.py @@ -564,28 +564,23 @@ class CLexer: if line[i] == ' ' or line[i] == '\t': i = i + 1 continue - o = ord(line[i]) - if (o >= 97 and o <= 122) or (o >= 65 and o <= 90) or \ - (o >= 48 and o <= 57): + if re.match(r"[a-zA-Z0-9]", line[i]): s = i while i < l: - o = ord(line[i]) - if (o >= 97 and o <= 122) or (o >= 65 and o <= 90) or \ - (o >= 48 and o <= 57) or \ - (" \t(){}:;,+-*/%&!|[]=><".find(line[i]) == -1): + if line[i] not in " \t(){}:;,+-*/%&!|[]=><": i = i + 1 else: break self.tokens.append(('name', line[s:i])) continue - if "(){}:;,[]".find(line[i]) != -1: + if line[i] in "(){}:;,[]": # if line[i] == '(' or line[i] == ')' or line[i] == '{' or \ # line[i] == '}' or line[i] == ':' or line[i] == ';' or \ # line[i] == ',' or line[i] == '[' or line[i] == ']': self.tokens.append(('sep', line[i])) i = i + 1 continue - if "+-*><=/%&!|.".find(line[i]) != -1: + if line[i] in "+-*><=/%&!|.": # if line[i] == '+' or line[i] == '-' or line[i] == '*' or \ # line[i] == '>' or line[i] == '<' or line[i] == '=' or \ # line[i] == '/' or line[i] == '%' or line[i] == '&' or \ @@ -597,8 +592,7 @@ class CLexer: continue j = i + 1 - if j < l and ( - "+-*><=/%&!|".find(line[j]) != -1): + if j < l and line[j] in "+-*><=/%&!|": # line[j] == '+' or line[j] == '-' or line[j] == '*' or \ # line[j] == '>' or line[j] == '<' or line[j] == '=' or \ # line[j] == '/' or line[j] == '%' or line[j] == '&' or \ @@ -611,10 +605,7 @@ class CLexer: continue s = i while i < l: - o = ord(line[i]) - if (o >= 97 and o <= 122) or (o >= 65 and o <= 90) or \ - (o >= 48 and o <= 57) or \ - (" \t(){}:;,+-*/%&!|[]=><".find(line[i]) == -1): + if line[i] not in " \t(){}:;,+-*/%&!|[]=><": # line[i] != ' ' and line[i] != '\t' and # line[i] != '(' and line[i] != ')' and # line[i] != '{' and line[i] != '}' and @@ -1555,10 +1546,8 @@ class CParser: if token is None: return token - while token[0] == "name" and ( - token[1] == "const" or \ - token[1] == "unsigned" or \ - token[1] == "signed"): + while (token[0] == "name" and + token[1] in ["const", "unsigned", "signed"]): if self.type == "": self.type = token[1] else: @@ -2402,8 +2391,7 @@ class docBuilder: pass typ = sorted(funcs.keys()) for type in typ: - if type == '' or type == 'void' or type == "int" or \ - type == "char *" or type == "const char *": + if type in ['', "void", "int", "char *", "const char *"]: continue output.write(" <type name='%s'>\n" % (type)) ids = funcs[type] @@ -2431,8 +2419,7 @@ class docBuilder: pass typ = sorted(funcs.keys()) for type in typ: - if type == '' or type == 'void' or type == "int" or \ - type == "char *" or type == "const char *": + if type in ['', "void", "int", "char *", "const char *"]: continue output.write(" <type name='%s'>\n" % (type)) ids = sorted(funcs[type]) -- 2.14.3

On Sat, Mar 17, 2018 at 02:23:27PM +0000, Radostin Stoyanov wrote:
Improve readability by reducing the complexity and length of conditional statements.
Example: The following condition:
if (o >= 97 and o <= 122) or (o >= 65 and o <= 90) or (o >= 48 and o <= 57) or (" \t(){}:;,+-*/%&!|[]=><".find(line[i]) == -1):
Will be True for every character that is not in string: " \t(){}:;,+-*/%&!|[]=><"
Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- docs/apibuild.py | 33 ++++++++++----------------------- 1 file changed, 10 insertions(+), 23 deletions(-)
diff --git a/docs/apibuild.py b/docs/apibuild.py index 644d96f69..1b9401226 100755 --- a/docs/apibuild.py +++ b/docs/apibuild.py @@ -564,28 +564,23 @@ class CLexer: if line[i] == ' ' or line[i] == '\t': i = i + 1 continue - o = ord(line[i]) - if (o >= 97 and o <= 122) or (o >= 65 and o <= 90) or \ - (o >= 48 and o <= 57): + if re.match(r"[a-zA-Z0-9]", line[i]):
Why not just use isalnum() function here - it will be massively faster than a regex match.
s = i while i < l: - o = ord(line[i]) - if (o >= 97 and o <= 122) or (o >= 65 and o <= 90) or \ - (o >= 48 and o <= 57) or \ - (" \t(){}:;,+-*/%&!|[]=><".find(line[i]) == -1): + if line[i] not in " \t(){}:;,+-*/%&!|[]=><": i = i + 1 else: break self.tokens.append(('name', line[s:i])) continue - if "(){}:;,[]".find(line[i]) != -1: + if line[i] in "(){}:;,[]": # if line[i] == '(' or line[i] == ')' or line[i] == '{' or \ # line[i] == '}' or line[i] == ':' or line[i] == ';' or \ # line[i] == ',' or line[i] == '[' or line[i] == ']': self.tokens.append(('sep', line[i])) i = i + 1 continue - if "+-*><=/%&!|.".find(line[i]) != -1: + if line[i] in "+-*><=/%&!|.": # if line[i] == '+' or line[i] == '-' or line[i] == '*' or \ # line[i] == '>' or line[i] == '<' or line[i] == '=' or \ # line[i] == '/' or line[i] == '%' or line[i] == '&' or \ @@ -597,8 +592,7 @@ class CLexer: continue
j = i + 1 - if j < l and ( - "+-*><=/%&!|".find(line[j]) != -1): + if j < l and line[j] in "+-*><=/%&!|": # line[j] == '+' or line[j] == '-' or line[j] == '*' or \ # line[j] == '>' or line[j] == '<' or line[j] == '=' or \ # line[j] == '/' or line[j] == '%' or line[j] == '&' or \ @@ -611,10 +605,7 @@ class CLexer: continue s = i while i < l: - o = ord(line[i]) - if (o >= 97 and o <= 122) or (o >= 65 and o <= 90) or \ - (o >= 48 and o <= 57) or \ - (" \t(){}:;,+-*/%&!|[]=><".find(line[i]) == -1): + if line[i] not in " \t(){}:;,+-*/%&!|[]=><": # line[i] != ' ' and line[i] != '\t' and # line[i] != '(' and line[i] != ')' and # line[i] != '{' and line[i] != '}' and @@ -1555,10 +1546,8 @@ class CParser: if token is None: return token
- while token[0] == "name" and ( - token[1] == "const" or \ - token[1] == "unsigned" or \ - token[1] == "signed"): + while (token[0] == "name" and + token[1] in ["const", "unsigned", "signed"]): if self.type == "": self.type = token[1] else: @@ -2402,8 +2391,7 @@ class docBuilder: pass typ = sorted(funcs.keys()) for type in typ: - if type == '' or type == 'void' or type == "int" or \ - type == "char *" or type == "const char *": + if type in ['', "void", "int", "char *", "const char *"]: continue output.write(" <type name='%s'>\n" % (type)) ids = funcs[type] @@ -2431,8 +2419,7 @@ class docBuilder: pass typ = sorted(funcs.keys()) for type in typ: - if type == '' or type == 'void' or type == "int" or \ - type == "char *" or type == "const char *": + if type in ['', "void", "int", "char *", "const char *"]: continue output.write(" <type name='%s'>\n" % (type)) ids = sorted(funcs[type])
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On 19/03/18 10:52, Daniel P. Berrangé wrote:
- o = ord(line[i]) - if (o >= 97 and o <= 122) or (o >= 65 and o <= 90) or \ - (o >= 48 and o <= 57): + if re.match(r"[a-zA-Z0-9]", line[i]): Why not just use isalnum() function here - it will be massively faster than a regex match.
Thanks, I didn't know about isalnum(). I will resend the patch.

PEP8 recommends not having spaces around = in a keyword argument or a default parameter value. https://www.python.org/dev/peps/pep-0008/#other-recommendations Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- docs/apibuild.py | 24 ++++++++++++------------ docs/index.py | 26 +++++++++++++------------- src/esx/esx_vi_generator.py | 4 ++-- tests/cputestdata/cpu-cpuid.py | 4 ++-- tests/cputestdata/cpu-reformat.py | 2 +- 5 files changed, 30 insertions(+), 30 deletions(-) diff --git a/docs/apibuild.py b/docs/apibuild.py index 1b9401226..8fecf5a81 100755 --- a/docs/apibuild.py +++ b/docs/apibuild.py @@ -137,8 +137,8 @@ def uniq(items): return k class identifier: - def __init__(self, name, header=None, module=None, type=None, lineno = 0, - info=None, extra=None, conditionals = None): + def __init__(self, name, header=None, module=None, type=None, lineno=0, + info=None, extra=None, conditionals=None): self.name = name self.header = header self.module = module @@ -209,7 +209,7 @@ class identifier: def get_conditionals(self): return self.conditionals - def update(self, header, module, type = None, info = None, extra=None, + def update(self, header, module, type=None, info=None, extra=None, conditionals=None): if self.name == debugsym and not quiet: print("=> update %s : %s" % (debugsym, (module, type, info, @@ -228,7 +228,7 @@ class identifier: self.set_conditionals(conditionals) class index: - def __init__(self, name = "noname"): + def __init__(self, name="noname"): self.name = name self.identifiers = {} self.functions = {} @@ -247,7 +247,7 @@ class index: warnings = warnings + 1 print(msg) - def add_ref(self, name, header, module, static, type, lineno, info=None, extra=None, conditionals = None): + def add_ref(self, name, header, module, static, type, lineno, info=None, extra=None, conditionals=None): if name[0:2] == '__': return None d = None @@ -269,7 +269,7 @@ class index: return d - def add(self, name, header, module, static, type, lineno, info=None, extra=None, conditionals = None): + def add(self, name, header, module, static, type, lineno, info=None, extra=None, conditionals=None): if name[0:2] == '__': return None d = None @@ -630,7 +630,7 @@ class CLexer: class CParser: """The C module parser""" - def __init__(self, filename, idx = None): + def __init__(self, filename, idx=None): self.filename = filename if len(filename) > 2 and filename[-2:] == '.h': self.is_header = 1 @@ -662,7 +662,7 @@ class CParser: def lineno(self): return self.lexer.getlineno() - def index_add(self, name, module, static, type, info=None, extra = None): + def index_add(self, name, module, static, type, info=None, extra=None): if self.is_header == 1: self.index.add(name, module, module, static, type, self.lineno(), info, extra, self.conditionals) @@ -671,7 +671,7 @@ class CParser: info, extra, self.conditionals) def index_add_ref(self, name, module, static, type, info=None, - extra = None): + extra=None): if self.is_header == 1: self.index.add_ref(name, module, module, static, type, self.lineno(), info, extra, self.conditionals) @@ -764,7 +764,7 @@ class CParser: # # Parse a comment block associate to a typedef # - def parseTypeComment(self, name, quiet = 0): + def parseTypeComment(self, name, quiet=0): if name[0:2] == '__': quiet = 1 @@ -809,7 +809,7 @@ class CParser: # # Parse a comment block associate to a macro # - def parseMacroComment(self, name, quiet = 0): + def parseMacroComment(self, name, quiet=0): global ignored_macros if name[0:2] == '__': @@ -886,7 +886,7 @@ class CParser: # parameters descriptions, finally returns a block as complete # as possible # - def mergeFunctionComment(self, name, description, quiet = 0): + def mergeFunctionComment(self, name, description, quiet=0): global ignored_functions if name == 'main': diff --git a/docs/index.py b/docs/index.py index 5224324b1..0d07ca4d0 100755 --- a/docs/index.py +++ b/docs/index.py @@ -144,7 +144,7 @@ def createTable(db, name): return -1 return ret -def checkTables(db, verbose = 1): +def checkTables(db, verbose=1): global TABLES if db is None: @@ -188,7 +188,7 @@ def checkTables(db, verbose = 1): pass return 0 -def openMySQL(db="libvir", passwd=None, verbose = 1): +def openMySQL(db="libvir", passwd=None, verbose=1): global DB if passwd is None: @@ -275,25 +275,25 @@ def updateSymbol(name, module, type, desc): return ret -def addFunction(name, module, desc = ""): +def addFunction(name, module, desc=""): return updateSymbol(name, module, 'function', desc) -def addMacro(name, module, desc = ""): +def addMacro(name, module, desc=""): return updateSymbol(name, module, 'macro', desc) -def addEnum(name, module, desc = ""): +def addEnum(name, module, desc=""): return updateSymbol(name, module, 'enum', desc) -def addStruct(name, module, desc = ""): +def addStruct(name, module, desc=""): return updateSymbol(name, module, 'struct', desc) -def addConst(name, module, desc = ""): +def addConst(name, module, desc=""): return updateSymbol(name, module, 'const', desc) -def addType(name, module, desc = ""): +def addType(name, module, desc=""): return updateSymbol(name, module, 'type', desc) -def addFunctype(name, module, desc = ""): +def addFunctype(name, module, desc=""): return updateSymbol(name, module, 'functype', desc) def addPage(resource, title): @@ -1055,7 +1055,7 @@ def analyzeHTMLPages(): import time -def getXMLDateArchive(t = None): +def getXMLDateArchive(t=None): if t is None: t = time.time() T = time.gmtime(t) @@ -1064,7 +1064,7 @@ def getXMLDateArchive(t = None): url = "http://www.redhat.com/archives/libvir-list/%d-%s/date.html" % (year, month) return url -def scanXMLMsgArchive(url, title, force = 0): +def scanXMLMsgArchive(url, title, force=0): if url is None or title is None: return 0 @@ -1094,7 +1094,7 @@ def scanXMLMsgArchive(url, title, force = 0): return 1 -def scanXMLDateArchive(t = None, force = 0): +def scanXMLDateArchive(t=None, force=0): global wordsDictArchive wordsDictArchive = {} @@ -1138,7 +1138,7 @@ def scanXMLDateArchive(t = None, force = 0): # Main code: open the DB, the API XML and analyze it # # # ######################################################################### -def analyzeArchives(t = None, force = 0): +def analyzeArchives(t=None, force=0): global wordsDictArchive ret = scanXMLDateArchive(t, force) diff --git a/src/esx/esx_vi_generator.py b/src/esx/esx_vi_generator.py index 0dca44fed..1e61cc71e 100755 --- a/src/esx/esx_vi_generator.py +++ b/src/esx/esx_vi_generator.py @@ -118,7 +118,7 @@ class Parameter(Member): return aligned(string, self.get_occurrence_comment() + "\n") - def generate_return(self, offset = 0, end_of_line = ";"): + def generate_return(self, offset=0, end_of_line=";"): if self.occurrence == OCCURRENCE__IGNORED: raise ValueError("invalid function parameter occurrence value '%s'" % self.occurrence) @@ -609,7 +609,7 @@ class Object(GenericObject): return source - def generate_deep_copy_code(self, add_banner = False): + def generate_deep_copy_code(self, add_banner=False): source = "" if self.extends is not None: diff --git a/tests/cputestdata/cpu-cpuid.py b/tests/cputestdata/cpu-cpuid.py index 43c92731a..c45118512 100755 --- a/tests/cputestdata/cpu-cpuid.py +++ b/tests/cputestdata/cpu-cpuid.py @@ -343,12 +343,12 @@ def convert(path): with open(path, "w") as f: json.dump({"return": {"model": {"name": "base", "props": props}}, "id": "model-expansion"}, - f, indent = 2, separators = (',', ': ')) + f, indent=2, separators=(',', ': ')) f.write("\n") for chunk in rest: f.write("\n") - json.dump(chunk, f, indent = 2, separators = (',', ': ')) + json.dump(chunk, f, indent=2, separators=(',', ': ')) f.write("\n") diff --git a/tests/cputestdata/cpu-reformat.py b/tests/cputestdata/cpu-reformat.py index 999ef1698..33976e775 100755 --- a/tests/cputestdata/cpu-reformat.py +++ b/tests/cputestdata/cpu-reformat.py @@ -5,5 +5,5 @@ import json dec = json.JSONDecoder() data, pos = dec.raw_decode(sys.stdin.read()) -json.dump(data, sys.stdout, indent = 2, separators = (',', ': ')) +json.dump(data, sys.stdout, indent=2, separators=(',', ': ')) print -- 2.14.3

On Sat, Mar 17, 2018 at 02:23:28PM +0000, Radostin Stoyanov wrote:
PEP8 recommends not having spaces around = in a keyword argument or a default parameter value.
https://www.python.org/dev/peps/pep-0008/#other-recommendations
Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- docs/apibuild.py | 24 ++++++++++++------------ docs/index.py | 26 +++++++++++++------------- src/esx/esx_vi_generator.py | 4 ++-- tests/cputestdata/cpu-cpuid.py | 4 ++-- tests/cputestdata/cpu-reformat.py | 2 +- 5 files changed, 30 insertions(+), 30 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Lists in Python are mutable and when used as a default value of a parameter for class constructor, its value will be shared between all class instances. Example: class Test: def __init__(self, mylist=[]): self.mylist = mylist A = Test() B = Test() A.mylist.append("mylist from instance A") print(B.mylist) # Will print ['mylist from instance A'] Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- src/hyperv/hyperv_wmi_generator.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/hyperv/hyperv_wmi_generator.py b/src/hyperv/hyperv_wmi_generator.py index f98a77562..acb9e5440 100755 --- a/src/hyperv/hyperv_wmi_generator.py +++ b/src/hyperv/hyperv_wmi_generator.py @@ -44,9 +44,9 @@ class WmiClass: to. """ - def __init__(self, name, versions = []): + def __init__(self, name, versions=None): self.name = name - self.versions = versions + self.versions = versions if versions else list() self.common = None -- 2.14.3

On Sat, Mar 17, 2018 at 02:23:29PM +0000, Radostin Stoyanov wrote:
Lists in Python are mutable and when used as a default value of a parameter for class constructor, its value will be shared between all class instances.
Example:
class Test: def __init__(self, mylist=[]): self.mylist = mylist
A = Test() B = Test() A.mylist.append("mylist from instance A") print(B.mylist) # Will print ['mylist from instance A']
Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- src/hyperv/hyperv_wmi_generator.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Use a set (unordered collections of unique elements) [1] to remove repeated elements in a list. 1: https://docs.python.org/3/tutorial/datastructures.html#sets Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- docs/apibuild.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/docs/apibuild.py b/docs/apibuild.py index 8fecf5a81..149cd41cc 100755 --- a/docs/apibuild.py +++ b/docs/apibuild.py @@ -130,11 +130,7 @@ def escape(raw): return raw def uniq(items): - d = {} - for item in items: - d[item] = 1 - k = sorted(d.keys()) - return k + return sorted(set(items)) class identifier: def __init__(self, name, header=None, module=None, type=None, lineno=0, -- 2.14.3

On Sat, Mar 17, 2018 at 02:23:30PM +0000, Radostin Stoyanov wrote:
Use a set (unordered collections of unique elements) [1] to remove repeated elements in a list.
1: https://docs.python.org/3/tutorial/datastructures.html#sets
Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- docs/apibuild.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

The uniq() function returns a sorted list, there is no need to sort this list again. Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- docs/apibuild.py | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/docs/apibuild.py b/docs/apibuild.py index 149cd41cc..400be124f 100755 --- a/docs/apibuild.py +++ b/docs/apibuild.py @@ -2319,8 +2319,7 @@ class docBuilder: if desc.find("DEPRECATED") != -1: output.write(" <deprecated/>\n") - ids = sorted(dict.macros.keys()) - for id in uniq(ids): + for id in uniq(dict.macros.keys()): # Macros are sometime used to masquerade other types. if id in dict.functions: continue @@ -2335,20 +2334,15 @@ class docBuilder: if id in dict.enums: continue output.write(" <exports symbol='%s' type='macro'/>\n" % (id)) - ids = sorted(dict.enums.keys()) - for id in uniq(ids): + for id in uniq(dict.enums.keys()): output.write(" <exports symbol='%s' type='enum'/>\n" % (id)) - ids = sorted(dict.typedefs.keys()) - for id in uniq(ids): + for id in uniq(dict.typedefs.keys()): output.write(" <exports symbol='%s' type='typedef'/>\n" % (id)) - ids = sorted(dict.structs.keys()) - for id in uniq(ids): + for id in uniq(dict.structs.keys()): output.write(" <exports symbol='%s' type='struct'/>\n" % (id)) - ids = sorted(dict.variables.keys()) - for id in uniq(ids): + for id in uniq(dict.variables.keys()): output.write(" <exports symbol='%s' type='variable'/>\n" % (id)) - ids = sorted(dict.functions.keys()) - for id in uniq(ids): + for id in uniq(dict.functions.keys()): output.write(" <exports symbol='%s' type='function'/>\n" % (id)) output.write(" </file>\n") @@ -2364,7 +2358,6 @@ class docBuilder: list(dict.typedefs.keys()) + \ list(dict.structs.keys()) + \ list(dict.enums.keys())) - ids.sort() for id in ids: output.write(" <ref name='%s'/>\n" % (id)) output.write(" </file>\n") -- 2.14.3

On Sat, Mar 17, 2018 at 02:23:31PM +0000, Radostin Stoyanov wrote:
The uniq() function returns a sorted list, there is no need to sort this list again.
Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- docs/apibuild.py | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Replace the print statement, that is only available in Py2, with a print function that is available in both Py2 and Py3 and drop the explicit python version in the shebang. Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- tests/cputestdata/cpu-reformat.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/cputestdata/cpu-reformat.py b/tests/cputestdata/cpu-reformat.py index 33976e775..d4ed8d811 100755 --- a/tests/cputestdata/cpu-reformat.py +++ b/tests/cputestdata/cpu-reformat.py @@ -1,4 +1,4 @@ -#!/usr/bin/env python2 +#!/usr/bin/env python import sys import json @@ -6,4 +6,4 @@ import json dec = json.JSONDecoder() data, pos = dec.raw_decode(sys.stdin.read()) json.dump(data, sys.stdout, indent=2, separators=(',', ': ')) -print +print("\n") -- 2.14.3

On Sat, Mar 17, 2018 at 02:23:32PM +0000, Radostin Stoyanov wrote:
Replace the print statement, that is only available in Py2, with a print function that is available in both Py2 and Py3 and drop the explicit python version in the shebang.
Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- tests/cputestdata/cpu-reformat.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Backslash between brackets in Python is redundant. [1] 1: https://lintlyci.github.io/Flake8Rules/rules/E502.html Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- docs/apibuild.py | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/docs/apibuild.py b/docs/apibuild.py index 400be124f..bcf7e5c24 100755 --- a/docs/apibuild.py +++ b/docs/apibuild.py @@ -385,8 +385,8 @@ class index: # check that function condition agrees with header if idx.functions[id].conditionals != \ self.functions[id].conditionals: - self.warning("Header condition differs from Function for %s:" \ - % id) + self.warning("Header condition differs from Function" + " for %s:" % id) self.warning(" H: %s" % self.functions[id].conditionals) self.warning(" C: %s" % idx.functions[id].conditionals) up = idx.functions[id] @@ -1883,7 +1883,7 @@ class CParser: raise Exception() except: self.error(("struct '%s' is not allowed to contain long " - "field '%s', use long long instead") \ + "field '%s', use long long instead") % (name, field[1])) # @@ -1940,8 +1940,8 @@ class CParser: if token[1] == "[": type = type + token[1] token = self.token() - while token is not None and (token[0] != "sep" or \ - token[1] != ";"): + while token is not None and (token[0] != "sep" or + token[1] != ";"): type = type + token[1] token = self.token() @@ -1955,9 +1955,9 @@ class CParser: token = self.parseBlock(token) else: self.comment = None - while token is not None and (token[0] != "sep" or \ - (token[1] != ';' and token[1] != ',')): - token = self.token() + while token is not None and (token[0] != "sep" or + token[1] not in ',;'): + token = self.token() self.comment = None if token is None or token[0] != "sep" or (token[1] != ';' and token[1] != ','): @@ -2352,11 +2352,11 @@ class docBuilder: module = self.modulename_file(file) output.write(" <file name='%s'>\n" % (module)) dict = self.headers[file] - ids = uniq(list(dict.functions.keys()) + \ - list(dict.variables.keys()) + \ - list(dict.macros.keys()) + \ - list(dict.typedefs.keys()) + \ - list(dict.structs.keys()) + \ + ids = uniq(list(dict.functions.keys()) + + list(dict.variables.keys()) + + list(dict.macros.keys()) + + list(dict.typedefs.keys()) + + list(dict.structs.keys()) + list(dict.enums.keys())) for id in ids: output.write(" <ref name='%s'/>\n" % (id)) -- 2.14.3

On Sat, Mar 17, 2018 at 02:23:33PM +0000, Radostin Stoyanov wrote:
Backslash between brackets in Python is redundant. [1]
1: https://lintlyci.github.io/Flake8Rules/rules/E502.html
Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- docs/apibuild.py | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

PEP8 recommends that the number of spaces used for indentation of Python code to be a multiple of four [1] [2]. 1: https://lintlyci.github.io/Flake8Rules/rules/E111.html 2: https://lintlyci.github.io/Flake8Rules/rules/E114.html Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- docs/apibuild.py | 298 ++++++++++++++++++++++++++++--------------------------- 1 file changed, 151 insertions(+), 147 deletions(-) diff --git a/docs/apibuild.py b/docs/apibuild.py index bcf7e5c24..9e73b4d27 100755 --- a/docs/apibuild.py +++ b/docs/apibuild.py @@ -248,11 +248,12 @@ class index: return None d = None try: - d = self.identifiers[name] - d.update(header, module, type, lineno, info, extra, conditionals) + d = self.identifiers[name] + d.update(header, module, type, lineno, info, extra, conditionals) except: - d = identifier(name, header, module, type, lineno, info, extra, conditionals) - self.identifiers[name] = d + d = identifier(name, header, module, type, lineno, info, extra, + conditionals) + self.identifiers[name] = d if d is not None and static == 1: d.set_static(1) @@ -265,16 +266,18 @@ class index: return d - def add(self, name, header, module, static, type, lineno, info=None, extra=None, conditionals=None): + def add(self, name, header, module, static, type, lineno, info=None, + extra=None, conditionals=None): if name[0:2] == '__': return None d = None try: - d = self.identifiers[name] - d.update(header, module, type, lineno, info, extra, conditionals) + d = self.identifiers[name] + d.update(header, module, type, lineno, info, extra, conditionals) except: - d = identifier(name, header, module, type, lineno, info, extra, conditionals) - self.identifiers[name] = d + d = identifier(name, header, module, type, lineno, info, extra, + conditionals) + self.identifiers[name] = d if d is not None and static == 1: d.set_static(1) @@ -308,93 +311,94 @@ class index: def merge(self, idx): for id in idx.functions.keys(): - # - # macro might be used to override functions or variables - # definitions - # - if id in self.macros: - del self.macros[id] - if id in self.functions: - self.warning("function %s from %s redeclared in %s" % ( + # + # macro might be used to override functions or variables + # definitions + # + if id in self.macros: + del self.macros[id] + if id in self.functions: + self.warning("function %s from %s redeclared in %s" % ( id, self.functions[id].header, idx.functions[id].header)) - else: - self.functions[id] = idx.functions[id] - self.identifiers[id] = idx.functions[id] + else: + self.functions[id] = idx.functions[id] + self.identifiers[id] = idx.functions[id] for id in idx.variables.keys(): - # - # macro might be used to override functions or variables - # definitions - # - if id in self.macros: - del self.macros[id] - if id in self.variables: - self.warning("variable %s from %s redeclared in %s" % ( + # + # macro might be used to override functions or variables + # definitions + # + if id in self.macros: + del self.macros[id] + if id in self.variables: + self.warning("variable %s from %s redeclared in %s" % ( id, self.variables[id].header, idx.variables[id].header)) - else: - self.variables[id] = idx.variables[id] - self.identifiers[id] = idx.variables[id] + else: + self.variables[id] = idx.variables[id] + self.identifiers[id] = idx.variables[id] for id in idx.structs.keys(): - if id in self.structs: - self.warning("struct %s from %s redeclared in %s" % ( + if id in self.structs: + self.warning("struct %s from %s redeclared in %s" % ( id, self.structs[id].header, idx.structs[id].header)) - else: - self.structs[id] = idx.structs[id] - self.identifiers[id] = idx.structs[id] + else: + self.structs[id] = idx.structs[id] + self.identifiers[id] = idx.structs[id] for id in idx.unions.keys(): - if id in self.unions: - print("union %s from %s redeclared in %s" % ( + if id in self.unions: + print("union %s from %s redeclared in %s" % ( id, self.unions[id].header, idx.unions[id].header)) - else: - self.unions[id] = idx.unions[id] - self.identifiers[id] = idx.unions[id] + else: + self.unions[id] = idx.unions[id] + self.identifiers[id] = idx.unions[id] for id in idx.typedefs.keys(): - if id in self.typedefs: - self.warning("typedef %s from %s redeclared in %s" % ( + if id in self.typedefs: + self.warning("typedef %s from %s redeclared in %s" % ( id, self.typedefs[id].header, idx.typedefs[id].header)) - else: - self.typedefs[id] = idx.typedefs[id] - self.identifiers[id] = idx.typedefs[id] + else: + self.typedefs[id] = idx.typedefs[id] + self.identifiers[id] = idx.typedefs[id] for id in idx.macros.keys(): - # - # macro might be used to override functions or variables - # definitions - # - if id in self.variables: - continue - if id in self.functions: - continue - if id in self.enums: - continue - if id in self.macros: - self.warning("macro %s from %s redeclared in %s" % ( + # + # macro might be used to override functions or variables + # definitions + # + if id in self.variables: + continue + if id in self.functions: + continue + if id in self.enums: + continue + if id in self.macros: + self.warning("macro %s from %s redeclared in %s" % ( id, self.macros[id].header, idx.macros[id].header)) - else: - self.macros[id] = idx.macros[id] - self.identifiers[id] = idx.macros[id] + else: + self.macros[id] = idx.macros[id] + self.identifiers[id] = idx.macros[id] for id in idx.enums.keys(): - if id in self.enums: - self.warning("enum %s from %s redeclared in %s" % ( + if id in self.enums: + self.warning("enum %s from %s redeclared in %s" % ( id, self.enums[id].header, idx.enums[id].header)) - else: - self.enums[id] = idx.enums[id] - self.identifiers[id] = idx.enums[id] + else: + self.enums[id] = idx.enums[id] + self.identifiers[id] = idx.enums[id] def merge_public(self, idx): for id in idx.functions.keys(): - if id in self.functions: - # check that function condition agrees with header - if idx.functions[id].conditionals != \ - self.functions[id].conditionals: - self.warning("Header condition differs from Function" - " for %s:" % id) - self.warning(" H: %s" % self.functions[id].conditionals) - self.warning(" C: %s" % idx.functions[id].conditionals) - up = idx.functions[id] - self.functions[id].update(None, up.module, up.type, up.info, up.extra) - # else: - # print("Function %s from %s is not declared in headers" % ( - # id, idx.functions[id].module)) - # TODO: do the same for variables. + if id in self.functions: + # check that function condition agrees with header + if idx.functions[id].conditionals != \ + self.functions[id].conditionals: + self.warning("Header condition differs from Function" + " for %s:" % id) + self.warning(" H: %s" % self.functions[id].conditionals) + self.warning(" C: %s" % idx.functions[id].conditionals) + up = idx.functions[id] + self.functions[id].update(None, up.module, up.type, up.info, + up.extra) + # else: + # print("Function %s from %s is not declared in headers" % ( + # id, idx.functions[id].module)) + # TODO: do the same for variables. def analyze_dict(self, type, dict): count = 0 @@ -726,7 +730,7 @@ class CParser: elif line[i] == '*': return line[:i] + line[i + 1:] else: - return line + return line return line def cleanupComment(self): @@ -877,11 +881,11 @@ class CParser: return((args, desc)) - # - # Parse a comment block and merge the information found in the - # parameters descriptions, finally returns a block as complete - # as possible - # + # + # Parse a comment block and merge the information found in the + # parameters descriptions, finally returns a block as complete + # as possible + # def mergeFunctionComment(self, name, description, quiet=0): global ignored_functions @@ -989,9 +993,9 @@ class CParser: desc = desc.strip() if quiet == 0: - # - # report missing comments - # + # + # report missing comments + # i = 0 while i < nbargs: if args[i][2] is None and args[i][0] != "void" and args[i][1] is not None: @@ -1023,7 +1027,7 @@ class CParser: if token is None: return None if token[0] == 'preproc': - # TODO macros with arguments + # TODO macros with arguments name = token[1] lst = [] token = self.lexer.token() @@ -1108,11 +1112,11 @@ class CParser: token = self.lexer.token() return token - # - # token acquisition on top of the lexer, it handle internally - # preprocessor and comments since they are logically not part of - # the program structure. - # + # + # token acquisition on top of the lexer, it handle internally + # preprocessor and comments since they are logically not part of + # the program structure. + # def push(self, tok): self.lexer.push(tok) @@ -1149,9 +1153,9 @@ class CParser: return token return None - # - # Parse a typedef, it records the type and its name. - # + # + # Parse a typedef, it records the type and its name. + # def parseTypedef(self, token): if token is None: return None @@ -1161,7 +1165,7 @@ class CParser: return None base_type = self.type type = base_type - #self.debug("end typedef type", token) + # self.debug("end typedef type", token) while token is not None: if token[0] == "name": name = token[1] @@ -1186,7 +1190,7 @@ class CParser: else: self.error("parsing typedef: expecting a name") return token - #self.debug("end typedef", token) + # self.debug("end typedef", token) if token is not None and token[0] == 'sep' and token[1] == ',': type = base_type token = self.token() @@ -1204,10 +1208,10 @@ class CParser: token = self.token() return token - # - # Parse a C code block, used for functions it parse till - # the balancing } included - # + # + # Parse a C code block, used for functions it parse till + # the balancing } included + # def parseBlock(self, token): while token is not None: if token[0] == "sep" and token[1] == "{": @@ -1243,27 +1247,27 @@ class CParser: token = self.token() return token - # - # Parse a C struct definition till the balancing } - # + # + # Parse a C struct definition till the balancing } + # def parseStruct(self, token): fields = [] - #self.debug("start parseStruct", token) + # self.debug("start parseStruct", token) while token is not None: if token[0] == "sep" and token[1] == "{": token = self.token() token = self.parseTypeBlock(token) elif token[0] == "sep" and token[1] == "}": self.struct_fields = fields - #self.debug("end parseStruct", token) - #print(fields) + # self.debug("end parseStruct", token) + # print(fields) token = self.token() return token else: base_type = self.type - #self.debug("before parseType", token) + # self.debug("before parseType", token) token = self.parseType(token) - #self.debug("after parseType", token) + # self.debug("after parseType", token) if token is not None and token[0] == "name": fname = token[1] token = self.token() @@ -1294,13 +1298,13 @@ class CParser: token = self.token() self.type = base_type self.struct_fields = fields - #self.debug("end parseStruct", token) - #print(fields) + # self.debug("end parseStruct", token) + # print(fields) return token - # - # Parse a C union definition till the balancing } - # + # + # Parse a C union definition till the balancing } + # def parseUnion(self, token): fields = [] # self.debug("start parseUnion", token) @@ -1348,9 +1352,9 @@ class CParser: # print(fields) return token - # - # Parse a C enum block, parse till the balancing } - # + # + # Parse a C enum block, parse till the balancing } + # def parseEnumBlock(self, token): self.enums = [] name = None @@ -1513,10 +1517,10 @@ class CParser: return token - # - # Parse a C definition block, used for structs or unions it parse till - # the balancing } - # + # + # Parse a C definition block, used for structs or unions it parse till + # the balancing } + # def parseTypeBlock(self, token): while token is not None: if token[0] == "sep" and token[1] == "{": @@ -1529,11 +1533,11 @@ class CParser: token = self.token() return token - # - # Parse a type: the fact that the type name can either occur after - # the definition or within the definition makes it a little harder - # if inside, the name token is pushed back before returning - # + # + # Parse a type: the fact that the type name can either occur after + # the definition or within the definition makes it a little harder + # if inside, the name token is pushed back before returning + # def parseType(self, token): self.type = "" self.struct_fields = [] @@ -1711,9 +1715,9 @@ class CParser: self.type = self.type + " " + token[1] token = self.token() - # - # if there is a parenthesis here, this means a function type - # + # + # if there is a parenthesis here, this means a function type + # if token is not None and token[0] == "sep" and token[1] == '(': self.type = self.type + token[1] token = self.token() @@ -1744,9 +1748,9 @@ class CParser: token = nametok return token - # - # do some lookahead for arrays - # + # + # do some lookahead for arrays + # if token is not None and token[0] == "name": nametok = token token = self.token() @@ -1766,7 +1770,7 @@ class CParser: self.error("parsing array type, ']' expected", token) return token elif token is not None and token[0] == "sep" and token[1] == ':': - # remove :12 in case it's a limited int size + # remove :12 in case it's a limited int size token = self.token() token = self.token() self.lexer.push(token) @@ -1774,9 +1778,9 @@ class CParser: return token - # - # Parse a signature: '(' has been parsed and we scan the type definition - # up to the ')' included + # + # Parse a signature: '(' has been parsed and we scan the type definition + # up to the ')' included def parseSignature(self, token): signature = [] if token is not None and token[0] == "sep" and token[1] == ')': @@ -1792,7 +1796,7 @@ class CParser: token = self.token() continue elif token is not None and token[0] == "sep" and token[1] == ')': - # only the type was provided + # only the type was provided if self.type == "...": signature.append((self.type, "...", None)) else: @@ -1886,10 +1890,10 @@ class CParser: "field '%s', use long long instead") % (name, field[1])) - # - # Parse a global definition, be it a type, variable or function - # the extern "C" blocks are a bit nasty and require it to recurse. - # + # + # Parse a global definition, be it a type, variable or function + # the extern "C" blocks are a bit nasty and require it to recurse. + # def parseGlobal(self, token): static = 0 if token[1] == 'extern': @@ -1946,9 +1950,9 @@ class CParser: token = self.token() if token is not None and token[0] == "op" and token[1] == "=": - # - # Skip the initialization of the variable - # + # + # Skip the initialization of the variable + # token = self.token() if token[0] == 'sep' and token[1] == '{': token = self.token() -- 2.14.3

On Sat, Mar 17, 2018 at 02:23:34PM +0000, Radostin Stoyanov wrote:
PEP8 recommends that the number of spaces used for indentation of Python code to be a multiple of four [1] [2].
1: https://lintlyci.github.io/Flake8Rules/rules/E111.html 2: https://lintlyci.github.io/Flake8Rules/rules/E114.html
Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- docs/apibuild.py | 298 ++++++++++++++++++++++++++++--------------------------- 1 file changed, 151 insertions(+), 147 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

The method strip_lead_star() removes a single leading asterisk character from a string by ignoring leading whitespace, otherwise it returns the original string. This could be achieved with a single if-statement followed by replace. Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- docs/apibuild.py | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/docs/apibuild.py b/docs/apibuild.py index 9e73b4d27..b914b1dce 100755 --- a/docs/apibuild.py +++ b/docs/apibuild.py @@ -722,15 +722,8 @@ class CParser: self.index.info = res def strip_lead_star(self, line): - l = len(line) - i = 0 - while i < l: - if line[i] == ' ' or line[i] == '\t': - i += 1 - elif line[i] == '*': - return line[:i] + line[i + 1:] - else: - return line + if line.lstrip().startswith('*'): + line = line.replace('*', '', 1) return line def cleanupComment(self): -- 2.14.3

On Sat, Mar 17, 2018 at 02:23:35PM +0000, Radostin Stoyanov wrote:
The method strip_lead_star() removes a single leading asterisk character from a string by ignoring leading whitespace, otherwise it returns the original string.
This could be achieved with a single if-statement followed by replace.
Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- docs/apibuild.py | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Improve readability and reduce complexity the method parseTypeComment(). Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- docs/apibuild.py | 42 ++++++++++++++++-------------------------- 1 file changed, 16 insertions(+), 26 deletions(-) diff --git a/docs/apibuild.py b/docs/apibuild.py index b914b1dce..fd1ee7958 100755 --- a/docs/apibuild.py +++ b/docs/apibuild.py @@ -757,48 +757,38 @@ class CParser: # # Parse a comment block associate to a typedef # - def parseTypeComment(self, name, quiet=0): + def parseTypeComment(self, name, quiet=False): if name[0:2] == '__': - quiet = 1 - - args = [] - desc = "" + quiet = True if self.comment is None: if not quiet: self.warning("Missing comment for type %s" % (name)) - return((args, desc)) - if self.comment[0] != '*': + return None + if not self.comment.startswith('*'): if not quiet: self.warning("Missing * in type comment for %s" % (name)) - return((args, desc)) + return None + lines = self.comment.split('\n') - if lines[0] == '*': - del lines[0] + # Remove lines that contain only single asterisk + lines[:] = [line for line in lines if line.strip() != '*'] + if lines[0] != "* %s:" % (name): if not quiet: self.warning("Misformatted type comment for %s" % (name)) self.warning(" Expecting '* %s:' got '%s'" % (name, lines[0])) - return((args, desc)) + return None del lines[0] - while len(lines) > 0 and lines[0] == '*': - del lines[0] - desc = "" - while len(lines) > 0: - l = lines[0] - while len(l) > 0 and l[0] == '*': - l = l[1:] - l = l.strip() - desc = desc + " " + l - del lines[0] - desc = desc.strip() + # Concatenate all remaining lines by striping leading asterisks + desc = " ".join([line.lstrip("*").strip() for line in lines]).strip() - if quiet == 0: - if desc == "": - self.warning("Type comment for %s lack description of the macro" % (name)) + if not (quiet or desc): + self.warning("Type comment for %s lack description of the macro" + % (name)) - return(desc) + return desc # # Parse a comment block associate to a macro # -- 2.14.3

On Sat, Mar 17, 2018 at 02:23:36PM +0000, Radostin Stoyanov wrote:
Improve readability and reduce complexity the method parseTypeComment().
Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- docs/apibuild.py | 42 ++++++++++++++++-------------------------- 1 file changed, 16 insertions(+), 26 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Reduce the number of if-statements used to assign a literals to corresponding class variables. Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- docs/apibuild.py | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/docs/apibuild.py b/docs/apibuild.py index fd1ee7958..29c89fd24 100755 --- a/docs/apibuild.py +++ b/docs/apibuild.py @@ -283,24 +283,19 @@ class index: d.set_static(1) if d is not None and name is not None and type is not None: - if type == "function": - self.functions[name] = d - elif type == "functype": - self.functions[name] = d - elif type == "variable": - self.variables[name] = d - elif type == "include": - self.includes[name] = d - elif type == "struct": - self.structs[name] = d - elif type == "union": - self.unions[name] = d - elif type == "enum": - self.enums[name] = d - elif type == "typedef": - self.typedefs[name] = d - elif type == "macro": - self.macros[name] = d + type_map = { + "function": self.functions, + "functype": self.functions, + "variable": self.variables, + "include": self.includes, + "struct": self.structs, + "union": self.unions, + "enum": self.enums, + "typedef": self.typedefs, + "macro": self.macros + } + if type in type_map: + type_map[type][name] = d else: self.warning("Unable to register type ", type) -- 2.14.3

On Sat, Mar 17, 2018 at 02:23:37PM +0000, Radostin Stoyanov wrote:
Reduce the number of if-statements used to assign a literals to corresponding class variables.
Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- docs/apibuild.py | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- docs/apibuild.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/docs/apibuild.py b/docs/apibuild.py index 29c89fd24..f9784f9bd 100755 --- a/docs/apibuild.py +++ b/docs/apibuild.py @@ -472,8 +472,7 @@ class CLexer: return None if line[0] == '#': - self.tokens = list(map((lambda x: ('preproc', x)), - line.split())) + self.tokens = [('preproc', word) for word in line.split()] # We might have whitespace between the '#' and preproc # macro name, so instead of having a single token element -- 2.14.3

On Sat, Mar 17, 2018 at 02:23:38PM +0000, Radostin Stoyanov wrote:
Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- docs/apibuild.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- docs/apibuild.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/apibuild.py b/docs/apibuild.py index f9784f9bd..24e0eb505 100755 --- a/docs/apibuild.py +++ b/docs/apibuild.py @@ -479,8 +479,8 @@ class CLexer: # of '#define' we might end up with '#' and 'define'. This # merges them back together if self.tokens[0][1] == "#": - self.tokens[0] = ('preproc', self.tokens[0][1] + self.tokens[1][1]) - self.tokens = self.tokens[:1] + self.tokens[2:] + self.tokens[0] = ('preproc', "#" + self.tokens[1][1]) + del self.tokens[1] break l = len(line) if line[0] == '"' or line[0] == "'": -- 2.14.3

On Sat, Mar 17, 2018 at 02:23:39PM +0000, Radostin Stoyanov wrote:
Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- docs/apibuild.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Improve readability and reduce the complexity of the code that is searching for string tokens (i.e. characters surrounded by a single or double quote). Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- docs/apibuild.py | 32 ++++++++++---------------------- 1 file changed, 10 insertions(+), 22 deletions(-) diff --git a/docs/apibuild.py b/docs/apibuild.py index 24e0eb505..f2971db5b 100755 --- a/docs/apibuild.py +++ b/docs/apibuild.py @@ -484,28 +484,16 @@ class CLexer: break l = len(line) if line[0] == '"' or line[0] == "'": - end = line[0] - line = line[1:] - found = 0 - tok = "" - while found == 0: - i = 0 - l = len(line) - while i < l: - if line[i] == end: - self.line = line[i+1:] - line = line[:i] - l = i - found = 1 - break - if line[i] == '\\': - i = i + 1 - i = i + 1 - tok = tok + line - if found == 0: - line = self.getline() - if line is None: - return None + quote = line[0] + i = 1 + while quote not in line[i:]: + i = len(line) + nextline = self.getline() + if nextline is None: + return None + line += nextline + + tok, self.line = line[1:].split(quote, 1) self.last = ('string', tok) return self.last -- 2.14.3

On Sat, Mar 17, 2018 at 02:23:40PM +0000, Radostin Stoyanov wrote:
Improve readability and reduce the complexity of the code that is searching for string tokens (i.e. characters surrounded by a single or double quote).
Signed-off-by: Radostin Stoyanov <rstoyanov1@gmail.com> --- docs/apibuild.py | 32 ++++++++++---------------------- 1 file changed, 10 insertions(+), 22 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On Sat, Mar 17, 2018 at 02:23:18PM +0000, Radostin Stoyanov wrote:
These patches improve the code style of python code by applying some PEP8 recommendations and simplifying some functions.
How did you identify all the places that needed fixing ? I'm assuming you were running via some automated tool. If so this series ought to ensure that the tool is run as part of "make syntax-check", so that we don't reintroduce the problems later. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On Mon, Mar 19, 2018 at 10:36:28AM +0000, Daniel P. Berrangé wrote:
On Sat, Mar 17, 2018 at 02:23:18PM +0000, Radostin Stoyanov wrote:
These patches improve the code style of python code by applying some PEP8 recommendations and simplifying some functions.
How did you identify all the places that needed fixing ? I'm assuming you were running via some automated tool. If so this series ought to ensure that the tool is run as part of "make syntax-check", so that we don't reintroduce the problems later.
Also, did you check whether the generated files are byte-for-byte identical before & after applying this patch series, under both py2 & py3 ? Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On 19/03/18 11:03, Daniel P. Berrangé wrote:
On Mon, Mar 19, 2018 at 10:36:28AM +0000, Daniel P. Berrangé wrote:
These patches improve the code style of python code by applying some PEP8 recommendations and simplifying some functions. How did you identify all the places that needed fixing ? I'm assuming you were running via some automated tool. If so this series ought to ensure
On Sat, Mar 17, 2018 at 02:23:18PM +0000, Radostin Stoyanov wrote: that the tool is run as part of "make syntax-check", so that we don't reintroduce the problems later. I found the PEP8 places that needed fixing with pylint/pycodestyle and while fixing them I noticed some other places that could have been optimised.
Also, did you check whether the generated files are byte-for-byte identical before & after applying this patch series, under both py2 & py3 ?
I checked the output of apibuild.py with: cd docs/ Then srcdir=. builddir=. /usr/bin/python2 ./apibuild.py; sha1sum *.xml | sha1sum and srcdir=. builddir=. /usr/bin/python3 ./apibuild.py; sha1sum *.xml | sha1sum Before and after applying the patches both commands returned the same hashsum (b068fa56c132fca35ba24302e0a394aa9f03be97) .
Regards, Daniel Radostin
participants (4)
-
Andrea Bolognani
-
Daniel P. Berrangé
-
Marc Hartmayer
-
Radostin Stoyanov