From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54841) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZegE3-0001tv-Ru for qemu-devel@nongnu.org; Wed, 23 Sep 2015 05:20:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZegDz-0002pj-M6 for qemu-devel@nongnu.org; Wed, 23 Sep 2015 05:20:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48449) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZegDz-0002pZ-Ef for qemu-devel@nongnu.org; Wed, 23 Sep 2015 05:20:27 -0400 From: Markus Armbruster References: <1442872682-6523-1-git-send-email-eblake@redhat.com> <1442872682-6523-3-git-send-email-eblake@redhat.com> <87r3lqh7u7.fsf@blackfin.pond.sub.org> <56016CA0.6080905@redhat.com> Date: Wed, 23 Sep 2015 11:20:23 +0200 In-Reply-To: <56016CA0.6080905@redhat.com> (Eric Blake's message of "Tue, 22 Sep 2015 08:58:40 -0600") Message-ID: <87wpvha3vs.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v5 02/46] qapi: Clean up qapi.py per pep8 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Michael Roth , marcandre.lureau@redhat.com, qemu-devel@nongnu.org, ehabkost@redhat.com, DirtY.iCE.hu@gmail.com Eric Blake writes: > On 09/22/2015 08:00 AM, Markus Armbruster wrote: >> Eric Blake 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 >>> --- >>> 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.