| Message ID | 20240820002318.1380276-3-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | move qapi under python/qemu/ | expand |
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.
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
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!
On Sat, Aug 31, 2024, 2:02 AM Markus Armbruster <armbru@redhat.com> wrote: > 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? > Yep, that's it exactly. > 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 > "WIP" it is. I'll adjust the conf and nix this patch. > > 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! > No problem! I know our tastes don't always match but I figure if we can agree on a config file, it'll make things nicer for the both of us in the future. We can discuss at KVM Forum too any of the little nits or preferences you'd like to see automated and enforced to unify the Python style we use in tree. I'd also like to discuss the usage of black formatting to automate away the formatting guesswork. It'd be a lot of churn, but it would drastically simplify things. (e.g. "make autoformat" in the python dir, plus we can look into emacs/vim/vscode config for the repo) I'll handle the config/maintenance/tooling but you can let me know your preferences. (This series should be a good example of which things I have configured for python/ that we didn't tackle for qapi. It isn't very much. Some things you prefer may now be *unhandled*, I think docstring length being confined to 72(?) or less might be one of them, I don't recall others at the moment.)
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,
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> --- python/setup.cfg | 5 +++++ scripts/qapi/commands.py | 2 +- scripts/qapi/events.py | 2 +- 3 files changed, 7 insertions(+), 2 deletions(-)