Re: [Qemu-devel] [PATCH v5 02/46] qapi: Clean up qapi.py per pep8

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Silence pep8, and make pylint a bit happier.  Just style cleanups;
> no semantic changes.

I had planned to clean it up after resolving the TODO fold into
QAPISchema, but I'm fine with doing it right away.  I think we'll want
to do a bit more for pylint, but limiting ourselves to really obvious
changes now makes sense.

> Signed-off-by: Eric Blake <address@hidden>
> ---
>  scripts/qapi.py | 165 
> ++++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 107 insertions(+), 58 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 06478bb..31c4bcc 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -71,6 +71,7 @@ all_names = {}
>  # Parsing the schema into expressions
>  #
>
> +
>  def error_path(parent):
>      res = ""
>      while parent:
> @@ -79,8 +80,10 @@ def error_path(parent):
>          parent = parent['parent']
>      return res
>
> +
>  class QAPISchemaError(Exception):
>      def __init__(self, schema, msg):
> +        Exception.__init__(self)

Not a style change.  One more below.  Separate patch?

>          self.fname = schema.fname
>          self.msg = msg
>          self.col = 1
> @@ -96,8 +99,10 @@ class QAPISchemaError(Exception):
>          return error_path(self.info) + \
>              "%s:%d:%d: %s" % (self.fname, self.line, self.col, self.msg)
>
> +
>  class QAPIExprError(Exception):
>      def __init__(self, expr_info, msg):
> +        Exception.__init__(self)
>          self.info = expr_info
>          self.msg = msg
>
> @@ -105,9 +110,10 @@ class QAPIExprError(Exception):
>          return error_path(self.info['parent']) + \
>              "%s:%d: %s" % (self.info['file'], self.info['line'], self.msg)
>
> +
>  class QAPISchemaParser(object):
>
> -    def __init__(self, fp, previously_included = [], incl_info = None):
> +    def __init__(self, fp, previously_included=[], incl_info=None):
>          abs_fname = os.path.abspath(fp.name)
>          fname = fp.name
>          self.fname = fname
> @@ -122,18 +128,18 @@ class QAPISchemaParser(object):
>          self.exprs = []
>          self.accept()
>
> -        while self.tok != None:
> +        while self.tok is not None:
>              expr_info = {'file': fname, 'line': self.line,
>                           'parent': self.incl_info}
>              expr = self.get_expr(False)
>              if isinstance(expr, dict) and "include" in expr:
>                  if len(expr) != 1:
> -                    raise QAPIExprError(expr_info, "Invalid 'include' 
> directive")
> +                    raise QAPIExprError(expr_info,
> +                                        "Invalid 'include' directive")
>                  include = expr["include"]
>                  if not isinstance(include, str):
> -                    raise QAPIExprError(expr_info,
> -                                        'Expected a file name (string), got: 
> %s'
> -                                        % include)
> +                    raise QAPIExprError(expr_info, 'Expected a file name '
> +                                        '(string), got: %s' % include)

I don't like breaking lines in the middle of an argument when another
argument shares a line with a part.  Perhaps:

                    raise QAPIExprError(expr_info,
                                'Expected a file name (string), got: %s'
                                % include)

>                  incl_abs_fname = os.path.join(os.path.dirname(abs_fname),
>                                                include)
>                  # catch inclusion cycle
[...]
> @@ -1292,6 +1323,7 @@ def camel_case(name):
>              new_name += ch.lower()
>      return new_name
>
> +
>  # ENUMName -> ENUM_NAME, EnumName1 -> ENUM_NAME1
>  # ENUM_NAME -> ENUM_NAME, ENUM_NAME1 -> ENUM_NAME1, ENUM_Name2 -> ENUM_NAME2
>  # ENUM24_Name -> ENUM24_NAME
> @@ -1308,12 +1340,13 @@ def camel_to_upper(value):
>          if c.isupper() and (i > 0) and c_fun_str[i - 1] != "_":
>              # Case 1: next string is lower
>              # Case 2: previous string is digit
> -            if (i < (l - 1) and c_fun_str[i + 1].islower()) or \
> -            c_fun_str[i - 1].isdigit():
> +            next_lower = i < (l - 1) and c_fun_str[i + 1].islower()
> +            if next_lower or c_fun_str[i - 1].isdigit():
>                  new_name += '_'
>          new_name += c

Dunno.  Perhaps:

            if i < (l - 1) and c_fun_str[i + 1].islower():
                new_name += '_'
            elif c_fun_str[i - 1].isdigit():
                new_name += '_'

Differently ugly, I guess.

The two comment lines are pretty worthless.

>      return new_name.lstrip('_').upper()
>
> +
>  def c_enum_const(type_name, const_name, prefix=None):
>      if prefix is not None:
>          type_name = prefix
[...]