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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> 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?

I understand you'd like to have some kind of stylized comment that
automated testing will reject, to serve as a "do not post before
resolving this issue" reminder.  Fair?

If yes, whatever floats your boat and doesn't interfere with existing
practice.

    $ git-grep -w FIXME | wc -l
    493
    $ git-grep -w TODO | wc -l
    1249
    $ git-grep -w XXX | wc -l
    78288
    $ git-grep -w WIP
    Binary file pc-bios/skiboot.lid matches

> 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.

Thanks!