[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 02/46] qapi: Clean up qapi.py per pep8
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v5 02/46] qapi: Clean up qapi.py per pep8 |
Date: |
Wed, 23 Sep 2015 11:20:23 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eric Blake <address@hidden> writes:
> On 09/22/2015 08:00 AM, Markus Armbruster wrote:
>> 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(-)
>>>
>
>>> +
>>> 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.
- Re: [Qemu-devel] [PATCH v5 03/46] qapi: Test for C member name collisions, (continued)
- Re: [Qemu-devel] [PATCH v5 03/46] qapi: Test for C member name collisions, Markus Armbruster, 2015/09/22
- Re: [Qemu-devel] [PATCH v5 03/46] qapi: Test for C member name collisions, Eric Blake, 2015/09/22
- Re: [Qemu-devel] [PATCH v5 03/46] qapi: Test for C member name collisions, Markus Armbruster, 2015/09/23
- Re: [Qemu-devel] [PATCH v5 03/46] qapi: Test for C member name collisions, Eric Blake, 2015/09/23
- Re: [Qemu-devel] [PATCH v5 03/46] qapi: Test for C member name collisions, Markus Armbruster, 2015/09/23
- Re: [Qemu-devel] [PATCH v5 03/46] qapi: Test for C member name collisions, Eric Blake, 2015/09/23
- Re: [Qemu-devel] [PATCH v5 03/46] qapi: Test for C member name collisions, Markus Armbruster, 2015/09/23
[Qemu-devel] [PATCH v5 02/46] qapi: Clean up qapi.py per pep8, Eric Blake, 2015/09/21
[Qemu-devel] [PATCH v5 07/46] qapi: Don't pass pre-existing error to later call, Eric Blake, 2015/09/21
Re: [Qemu-devel] [PATCH v5 07/46] qapi: Don't pass pre-existing error to later call, Eric Blake, 2015/09/26
Re: [Qemu-devel] [PATCH v5 07/46] qapi: Don't pass pre-existing error to later call, Eric Blake, 2015/09/26
Re: [Qemu-devel] [PATCH v5 07/46] qapi: Don't pass pre-existing error to later call, Markus Armbruster, 2015/09/28
[Qemu-devel] [PATCH v5 08/46] qapi: Reuse code for flat union base validation, Eric Blake, 2015/09/21