Eric Blake <ebl...@redhat.com> writes: > 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.
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.
