John Snow <js...@redhat.com> writes:
> On Fri, Aug 30, 2024 at 7:09 AM Markus Armbruster <arm...@redhat.com> wrote:
>
>> John Snow <js...@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 <js...@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!
