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

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

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

I figure that'll be easier than explaining the fixing the commit message
not to claim "just style" ;)

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

I appreciate less churn, but I'm aware of diminishing returns.
Reshuffling may be more trouble than it's worth.  Your call.

>>> @@ -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.

Yes, please.