Re: [PATCH 2/8] python/qapi: change "FIXME" to "TODO"

[John Snow <jsnow@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4530 bytes --]

On Fri, Aug 30, 2024 at 7:09 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> 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 <jsnow@redhat.com>
>
> 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=yes
> >  # TODO: Remove after we opt in to Pylint 2.8.3. See commit msg.
> >  min-similarity-lines=6
> >
> > +[pylint.miscellaneous]
> > +
> > +# forbid FIXME/XXX comments, allow TODO.
> > +notes=FIXME,
> > +      XXX,
> >
> >  [isort]
> >  force_grid_wrap=4
> > 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

[-- Attachment #2: Type: text/html, Size: 5732 bytes --]