From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 8B544CA100C for ; Fri, 30 Aug 2024 18:34:29 +0000 (UTC) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1sk6RE-0006Mm-AB; Fri, 30 Aug 2024 14:33:36 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1sk6RC-0006MG-3W for qemu-devel@nongnu.org; Fri, 30 Aug 2024 14:33:34 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.129.124]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1sk6R8-0000fc-0y for qemu-devel@nongnu.org; Fri, 30 Aug 2024 14:33:32 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1725042807; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=wWL8wwbc/ToTNjsa6egkPGLfmYjcUGxF4bYkrx6xB34=; b=H0JiORZ64FNsBlipRVvmPSZnG9F+bfLH3u9eoYKA6H0xPPyW92hPVRILkOh3r0tCeWQPRN c/bJI5nTOOifKJeJV8KCdhbM/g0kbxlSFK9t3I1WdrNS2x1N/M8PHyT5aqCBJpy3AfQ9YT 9wG3BsFF/5oaOc4RuWqixiFTX50XfHw= Received: from mail-pj1-f70.google.com (mail-pj1-f70.google.com [209.85.216.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-635-7MnBoV9hPgCGGmjH9ti1jw-1; Fri, 30 Aug 2024 14:33:25 -0400 X-MC-Unique: 7MnBoV9hPgCGGmjH9ti1jw-1 Received: by mail-pj1-f70.google.com with SMTP id 98e67ed59e1d1-2d3ce4f35c1so1937196a91.1 for ; Fri, 30 Aug 2024 11:33:24 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1725042804; x=1725647604; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=wWL8wwbc/ToTNjsa6egkPGLfmYjcUGxF4bYkrx6xB34=; b=Ff44fv0oqcAEXGed4jBrjeI9nT2Ul6oSeLLY0OG4nLc6rOn+Ts3nEsa2tPpEznIvR1 zdJXPWrZOkLrh7yAmuLAf9PFqRIiHr0vp4Z5c9NfAJC+m5mAJjbz/8MZ5pbj6V/rXDvg BTwv6wwNzmAh79EG4VX491yN4e+rFTZhOHuDUBeKRMkU/AFfF7hZUH4WwWPOUxjK99Wp +cGhHjU7h9TXv4yQFxSwc8vBonmjnCi6rQyNRLfM8vsq9qquVefdPRgJExNfcl8RajkI nKZfEtAOu5lVCHs6zdscdkOdjnAwGCh3l67aqL5aZQfmZz2mjcA/GVwRy1h/vYBJa7Vc jisg== X-Gm-Message-State: AOJu0YzhCMBsothJ2RM3i/G90nN8HIWpPMUG1FKdBtbOvCivtIzXFYCH FBADvKnTO1jQe2PHw8ZR3ihtQAK5tbUFgTdHnBIPpExj+JVJuLT8PXr1fzNuXTes5td1JlYNqt+ QJaAAzocZcP/mHr1upGWt7paFgletCheTjzWj2AKHilX9cyLX3Q1vv2+UolFVnpaGf3LscXN6XG KkFY4Sb2w/eqgP+fGru7tn5D/OqE8= X-Received: by 2002:a17:90a:b00a:b0:2cb:5dbb:d394 with SMTP id 98e67ed59e1d1-2d856a2ea26mr10207753a91.4.1725042804020; Fri, 30 Aug 2024 11:33:24 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFgWKfO3PFTu/2GQ5fD0DH5SrXYZxyKUT6zCaY+LY6PCj4/XN806irVCE+D7b57VnclGXubRxL+SNS9OC7ZtQI= X-Received: by 2002:a17:90a:b00a:b0:2cb:5dbb:d394 with SMTP id 98e67ed59e1d1-2d856a2ea26mr10207704a91.4.1725042803554; Fri, 30 Aug 2024 11:33:23 -0700 (PDT) MIME-Version: 1.0 References: <20240820002318.1380276-1-jsnow@redhat.com> <20240820002318.1380276-3-jsnow@redhat.com> <87ikvicln3.fsf@pond.sub.org> In-Reply-To: <87ikvicln3.fsf@pond.sub.org> From: John Snow Date: Fri, 30 Aug 2024 14:33:12 -0400 Message-ID: Subject: Re: [PATCH 2/8] python/qapi: change "FIXME" to "TODO" To: Markus Armbruster Cc: qemu-devel@nongnu.org, Cleber Rosa , =?UTF-8?B?TWFyYy1BbmRyw6kgTHVyZWF1?= , Thomas Huth , Peter Maydell , Beraldo Leal , Michael Roth , Paolo Bonzini , =?UTF-8?Q?Daniel_P=2E_Berrang=C3=A9?= , =?UTF-8?Q?Philippe_Mathieu=2DDaud=C3=A9?= Content-Type: multipart/alternative; boundary="0000000000008762050620ead3cb" Received-SPF: pass client-ip=170.10.129.124; envelope-from=jsnow@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.001, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H3=0.001, RCVD_IN_MSPIKE_WL=0.001, RCVD_IN_VALIDITY_CERTIFIED_BLOCKED=0.001, RCVD_IN_VALIDITY_RPBL_BLOCKED=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org --0000000000008762050620ead3cb Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, Aug 30, 2024 at 7:09=E2=80=AFAM Markus Armbruster wrote: > John Snow writes: > > > qemu.git/python/setup.cfg disallows checking in any code with "XXX", > > "FIXME" or "TODO" in the comments. Soften the restriction to only > > prohibit "FIXME", and change the two occurrences of "FIXME" in qapi to > > read "TODO" instead. > > > > Signed-off-by: John Snow > > I don't like forbidding FIXME comments. It's as futile as forbidding > known bugs. All it can accomplish is making people use other, and > likely less clear ways to document them. > > Perhaps projects exist that use FIXME comments only for known bugs in > uncommitted code. To me, that feels *nuts*. I commit all kinds of crap > in my tree. I don't need silly "make check" failures while I develop, > the non-silly ones cause enough friction already. > > In fact, we're quite happy to use FIXME comments even in merged code: > > $ git-grep FIXME | wc -l > 494 > > I can't see why python/ should be different. > > > --- > > python/setup.cfg | 5 +++++ > > scripts/qapi/commands.py | 2 +- > > scripts/qapi/events.py | 2 +- > > 3 files changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/python/setup.cfg b/python/setup.cfg > > index 3b4e2cc5501..72b58c98c99 100644 > > --- a/python/setup.cfg > > +++ b/python/setup.cfg > > @@ -169,6 +169,11 @@ ignore-signatures=3Dyes > > # TODO: Remove after we opt in to Pylint 2.8.3. See commit msg. > > min-similarity-lines=3D6 > > > > +[pylint.miscellaneous] > > + > > +# forbid FIXME/XXX comments, allow TODO. > > +notes=3DFIXME, > > + XXX, > > > > [isort] > > force_grid_wrap=3D4 > > diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py > > index 79951a841f5..cffed6cd3ba 100644 > > --- a/scripts/qapi/commands.py > > +++ b/scripts/qapi/commands.py > > @@ -385,7 +385,7 @@ def visit_command(self, > > coroutine: bool) -> None: > > if not gen: > > return > > - # FIXME: If T is a user-defined type, the user is responsible > > + # TODO: If T is a user-defined type, the user is responsible > > # for making this work, i.e. to make T's condition the > > # conjunction of the T-returning commands' conditions. If T > > # is a built-in type, this isn't possible: the > > diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py > > index d1f639981a9..36dc0c50c78 100644 > > --- a/scripts/qapi/events.py > > +++ b/scripts/qapi/events.py > > @@ -84,7 +84,7 @@ def gen_event_send(name: str, > > boxed: bool, > > event_enum_name: str, > > event_emit: str) -> str: > > - # FIXME: Our declaration of local variables (and of 'errp' in the > > + # TODO: Our declaration of local variables (and of 'errp' in the > > # parameter list) can collide with exploded members of the event's > > # data type passed in as parameters. If this collision ever hits = in > > # practice, we can rename our local variables with a leading _ > prefix, > > Starting a comment with TODO tells me there's work to do. > > Starting it with FIXME tells me there's a bug to fix. That's more > specific. Replacing FIXME by TODO loses information. > meh. I do use the "prohibit fixme" personally because I've the memory of a goldfish and I like setting up bombs for myself when I run tests, but willing to cede if it gets me what I want otherwise. I could be coerced to using "XXX" as my WIP testing bomb. Or maybe literally just adding "WIP" as a new bomb. Is that a fair trade? There are likely other standards differences between the two subtrees, potentially things like documentation string length and so on -- I invite you to take a look at the setup.cfg file and tweak things to your liking and run "make check-minreqs" to see what barks, if anything. After you run that command, you can type "source .dev-venv/bin/activate.sh" (or .fish or whatever) and then "pylint --generate-rcfile | less" to get a sample config and see all of the buttons, knobs and levers you could pull. You can leave the environment when you're done with "deactivate". Mentioning this only because there have been times in the past that my formatting hasn't been to your liking, but there are avenues to programmatically enforce it to make my qapi patches nicer for your tastes in the future. --js --0000000000008762050620ead3cb Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Fri, Aug 30, 2024 at 7:09=E2=80=AF= AM Markus Armbruster <armbru@redhat= .com> wrote:
John Snow <jsn= ow@redhat.com> writes:

> qemu.git/python/setup.cfg disallows checking in any code with "XX= X",
> "FIXME" or "TODO" in the comments. Soften the rest= riction to only
> prohibit "FIXME", and change the two occurrences of "FI= XME" in qapi to
> read "TODO" instead.
>
> Signed-off-by: John Snow <jsnow@redhat.com>

I don't like forbidding FIXME comments.=C2=A0 It's as futile as for= bidding
known bugs.=C2=A0 All it can accomplish is making people use other, and
likely less clear ways to document them.

Perhaps projects exist that use FIXME comments only for known bugs in
uncommitted code.=C2=A0 To me, that feels *nuts*.=C2=A0 I commit all kinds = of crap
in my tree.=C2=A0 I don't need silly "make check" failures wh= ile I develop,
the non-silly ones cause enough friction already.

In fact, we're quite happy to use FIXME comments even in merged code:
=C2=A0 =C2=A0 $ git-grep FIXME | wc -l
=C2=A0 =C2=A0 494

I can't see why python/ should be different.

> ---
>=C2=A0 python/setup.cfg=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| 5 +++++
>=C2=A0 scripts/qapi/commands.py | 2 +-
>=C2=A0 scripts/qapi/events.py=C2=A0 =C2=A0| 2 +-
>=C2=A0 3 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/python/setup.cfg b/python/setup.cfg
> index 3b4e2cc5501..72b58c98c99 100644
> --- a/python/setup.cfg
> +++ b/python/setup.cfg
> @@ -169,6 +169,11 @@ ignore-signatures=3Dyes
>=C2=A0 # TODO: Remove after we opt in to Pylint 2.8.3. See commit msg.<= br> >=C2=A0 min-similarity-lines=3D6
>=C2=A0
> +[pylint.miscellaneous]
> +
> +# forbid FIXME/XXX comments, allow TODO.
> +notes=3DFIXME,
> +=C2=A0 =C2=A0 =C2=A0 XXX,
>=C2=A0
>=C2=A0 [isort]
>=C2=A0 force_grid_wrap=3D4
> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> index 79951a841f5..cffed6cd3ba 100644
> --- a/scripts/qapi/commands.py
> +++ b/scripts/qapi/commands.py
> @@ -385,7 +385,7 @@ def visit_command(self,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 coroutine: bool) -> None:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if not gen:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 # FIXME: If T is a user-defined type, the= user is responsible
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 # TODO: If T is a user-defined type, the = user is responsible
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # for making this work, i.e. to make= T's condition the
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # conjunction of the T-returning com= mands' conditions.=C2=A0 If T
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 # is a built-in type, this isn't= possible: the
> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
> index d1f639981a9..36dc0c50c78 100644
> --- a/scripts/qapi/events.py
> +++ b/scripts/qapi/events.py
> @@ -84,7 +84,7 @@ def gen_event_send(name: str,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0boxed: bool,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0event_enum_name: str,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0event_emit: str) -> str:
> -=C2=A0 =C2=A0 # FIXME: Our declaration of local variables (and of = 9;errp' in the
> +=C2=A0 =C2=A0 # TODO: Our declaration of local variables (and of '= ;errp' in the
>=C2=A0 =C2=A0 =C2=A0 # parameter list) can collide with exploded member= s of the event's
>=C2=A0 =C2=A0 =C2=A0 # data type passed in as parameters.=C2=A0 If this= collision ever hits in
>=C2=A0 =C2=A0 =C2=A0 # practice, we can rename our local variables with= a leading _ prefix,

Starting a comment with TODO tells me there's work to do.

Starting it with FIXME tells me there's a bug to fix.=C2=A0 That's = more
specific.=C2=A0 Replacing FIXME by TODO loses information.
=

meh. I do use the "prohibit fixme" personally= because I've the memory of a goldfish and I like setting up bombs for = myself when I run tests, but willing to cede if it gets me what I want othe= rwise. I could be coerced to using "XXX" as my WIP testing bomb. = Or maybe literally just adding "WIP" as a new bomb. Is that a fai= r trade?

There are likely other standards diff= erences between the two subtrees, potentially things like documentation str= ing length and so on -- I invite you to take a look at the setup.cfg file a= nd tweak things to your liking and run "make check-minreqs" to se= e what barks, if anything.

After you run that comm= and, you can type "source .dev-venv/bin/activate.sh" (or .fish or= whatever) and then "pylint --generate-rcfile | less" to get a sa= mple config and see all of the buttons, knobs and levers you could pull. Yo= u can leave the environment when you're done with "deactivate"= ;.

Mentioning this only because there have bee= n times in the past that my formatting hasn't been to your liking, but = there are avenues to programmatically enforce it to make my qapi patches ni= cer for your tastes in the future.

--js
<= /div>
--0000000000008762050620ead3cb--