From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33685) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZeP1r-0002Kt-Pa for qemu-devel@nongnu.org; Tue, 22 Sep 2015 10:58:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZeP1q-00049h-Ly for qemu-devel@nongnu.org; Tue, 22 Sep 2015 10:58:47 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45581) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZeP1q-00049V-FU for qemu-devel@nongnu.org; Tue, 22 Sep 2015 10:58:46 -0400 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> From: Eric Blake Message-ID: <56016CA0.6080905@redhat.com> Date: Tue, 22 Sep 2015 08:58:40 -0600 MIME-Version: 1.0 In-Reply-To: <87r3lqh7u7.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="t6D4poBwM1QLW2VkHs9t3LXpjq2P69SIS" 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: Markus Armbruster Cc: marcandre.lureau@redhat.com, DirtY.iCE.hu@gmail.com, qemu-devel@nongnu.org, ehabkost@redhat.com, Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --t6D4poBwM1QLW2VkHs9t3LXpjq2P69SIS Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 09/22/2015 08:00 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> Silence pep8, and make pylint a bit happier. Just style cleanups; >> no semantic changes. >=20 > 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. >=20 >> 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) >=20 > 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 n= ame ' >> + '(string), got: %s' % include= ) >=20 > I don't like breaking lines in the middle of an argument when another > argument shares a line with a part. Perhaps: >=20 > raise QAPIExprError(expr_info, > 'Expected a file name (string), got: %s= ' > % include) Hmm - I touch the same lines again in patch 6: | include =3D 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] !=3D "_": >> # 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 =3D i < (l - 1) and c_fun_str[i + 1].islower()= >> + if next_lower or c_fun_str[i - 1].isdigit(): >> new_name +=3D '_' >> new_name +=3D c >=20 > Dunno. Perhaps: >=20 > if i < (l - 1) and c_fun_str[i + 1].islower(): > new_name +=3D '_' > elif c_fun_str[i - 1].isdigit(): > new_name +=3D '_' >=20 > 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 +=3D '_' or maybe reverse the conditional: Your suggestion looks the least bad to me. >=20 > The two comment lines are pretty worthless. I can drop them if you'd like. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --t6D4poBwM1QLW2VkHs9t3LXpjq2P69SIS Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJWAWygAAoJEKeha0olJ0NqtisH/2WKm7sCOa3oC8K02RVuVcK5 CNrqMvAmiRUTggKtquJoL8VU3YfLs3Rfs8TKRXsM/HCeiii+N3pfkJ9noLgq4qcB 5wx/p1kg4QHptgP3nSIUIg+g9IVhTVjGfFystaJtZa3do7fh2ieImAlhQrnp/+3+ EU+ua419W4NAWFHNbBAX8zbbSW3xmGWt13Yb7K1rTNRCAR8na2NDvplyN9Fpc4W6 Qt7vt6tRrZxwl7St8XByiEZPPbleUW7xWxuCl69gz64R7gD/57ZxgvV6bG1Agxut YD1JGRmp53CTQeuXVrUkLYqr7UnPb/35AqVOsyEIykWqSnJ83A3jx6pux+se0Ck= =ynha -----END PGP SIGNATURE----- --t6D4poBwM1QLW2VkHs9t3LXpjq2P69SIS--