Re: [Qemu-devel] [PATCH v6 04/16] 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,
> plus killing a useless comment in camel_to_upper(); no semantic
> changes.
>
> Signed-off-by: Eric Blake <address@hidden>
[...]
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index a2d869e..7e8a99d 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
[...]
> @@ -667,22 +695,23 @@ def check_keys(expr_elem, meta, required, optional=[]):
>      if not isinstance(name, str):
>          raise QAPIExprError(info,
>                              "'%s' key must have a string value" % meta)
> -    required = required + [ meta ]
> +    required = required + [meta]
>      for (key, value) in expr.items():
> -        if not key in required and not key in optional:
> +        if key not in required + optional:

The purely mechanical cleanup would be

        if key not in required and key not in optional:

Yours is a bit more concise, but I'm not sure it's actually easier to
read.  It's also less efficient unless a sufficiently smart compiler
transforms it back.  Not that efficiency matters here.

>              raise QAPIExprError(info,
>                                  "Unknown key '%s' in %s '%s'"
>                                  % (key, meta, name))
> -        if (key == 'gen' or key == 'success-response') and value != False:
> +        if (key == 'gen' or key == 'success-response') and value is not 
> False:
>              raise QAPIExprError(info,
>                                  "'%s' of %s '%s' should only use false value"
>                                  % (key, meta, name))
>      for key in required:
> -        if not expr.has_key(key):
> +        if key not in expr:
>              raise QAPIExprError(info,
>                                  "Key '%s' is missing from %s '%s'"
>                                  % (key, meta, name))
>
> +
>  def check_exprs(exprs):
>      global all_names
>
[...]
> @@ -1293,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
> @@ -1307,14 +1338,14 @@ def camel_to_upper(value):
>          c = c_fun_str[i]
>          # When c is upper and no "_" appears before, do more checks
>          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():
> +            if i < (l - 1) and c_fun_str[i + 1].islower():

I doubt we need the parenthesis around (l - 1).

> +                new_name += '_'
> +            elif c_fun_str[i - 1].isdigit():
>                  new_name += '_'
>          new_name += c
>      return new_name.lstrip('_').upper()
>
> +
>  def c_enum_const(type_name, const_name, prefix=None):
>      if prefix is not None:
>          type_name = prefix
[...]

Happy to touch up on commit to my tree.