[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: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v5 02/46] qapi: Clean up qapi.py per pep8 |
Date: |
Tue, 22 Sep 2015 08:58:40 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 |
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.
>> 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
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v5 03/46] qapi: Test for C member name collisions, (continued)
- [Qemu-devel] [PATCH v5 03/46] qapi: Test for C member name collisions, Eric Blake, 2015/09/21
- 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