On 09/22/2015 08:00 AM, Markus Armbruster wrote: > Eric Blake <ebl...@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 <ebl...@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
signature.asc
Description: OpenPGP digital signature