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

[Markus Armbruster <armbru@redhat.com>]

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.