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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3560 bytes --]

On 09/22/2015 08:00 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com>
>> ---
>>  scripts/qapi.py | 165 ++++++++++++++++++++++++++++++++++++--------------------
>>  1 file changed, 107 insertions(+), 58 deletions(-)
>>

>> +
>>  class QAPISchemaError(Exception):
>>      def __init__(self, schema, msg):
>> +        Exception.__init__(self)
> 
> Not a style change.  One more below.  Separate patch?

pep8 didn't mind, but pylint did.  Personally, I don't know what happens
if you don't call the superclass constructor.  But since pep8 didn't
flag it, I can agree to split into a separate patch.


>>                  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)

Hmm - I touch the same lines again in patch 6:

|      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 string for 'include'")

Looks like I should reshuffle the series to avoid the churn.

>> @@ -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 complaints from pep8 were the \ continuation, and the fact that the
continued if condition was at the same indentation as the body of the
if; another ugly solution would be another layer of ():

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

or maybe reverse the conditional:

Your suggestion looks the least bad to me.

> 
> The two comment lines are pretty worthless.

I can drop them if you'd like.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]