[PATCH 00/10] qapi: misc testing and doc patches

[PATCH 00/10] qapi: misc testing and doc patches

[John Snow]

GitLab: https://gitlab.com/jsnow/qemu/-/pipelines/1684045409

This is a series of random qapi and documentation patches;

Patches 1-3: Formalize QAPI python testing (finally ...)
Patches 4-10: Miscellaneous patches and fixes related to the
documentation overhaul project; but were standalone enough that they
could be excised from a fairly lengthy series ...

Markus: I think everything here is suitable for merging, even though
some of it might lack a little context. The more I pull out of that big
series, the easier that series will be to review :)

Please stage as much as you feel comfortable doing, you can skip any
individual patch at your discretion.

John Snow (10):
  qapi: update pylintrc config
  python: add qapi static analysis tests
  qapi: delete un-needed python static analysis configs
  docs/qapidoc: support header-less freeform sections
  qapi/parser: adjust info location for doc body section
  docs/qapidoc: remove example section support
  qapi: expand tags to all doc sections
  qapi/schema: add __repr__ to QAPIDoc.Section
  qapi/source: allow multi-line QAPISourceInfo advancing
  docs: disambiguate cross-references

 docs/devel/codebase.rst        |   6 +-
 docs/glossary.rst              |  10 +--
 docs/sphinx/qapidoc.py         |  18 ++---
 python/tests/qapi-flake8.sh    |   2 +
 python/tests/qapi-isort.sh     |   2 +
 python/tests/qapi-mypy.sh      |   2 +
 python/tests/qapi-pylint.sh    |   4 ++
 scripts/qapi/.flake8           |   3 -
 scripts/qapi/.isort.cfg        |   7 --
 scripts/qapi/mypy.ini          |   4 --
 scripts/qapi/parser.py         | 124 +++++++++++++++++++++++++--------
 scripts/qapi/pylintrc          |   1 +
 scripts/qapi/source.py         |   4 +-
 tests/qapi-schema/doc-good.out |  10 +--
 tests/qapi-schema/test-qapi.py |   2 +-
 15 files changed, 128 insertions(+), 71 deletions(-)
 create mode 100755 python/tests/qapi-flake8.sh
 create mode 100755 python/tests/qapi-isort.sh
 create mode 100755 python/tests/qapi-mypy.sh
 create mode 100755 python/tests/qapi-pylint.sh
 delete mode 100644 scripts/qapi/.flake8
 delete mode 100644 scripts/qapi/.isort.cfg
 delete mode 100644 scripts/qapi/mypy.ini

-- 
2.48.1

[PATCH 01/10] qapi: update pylintrc config

[John Snow]

If you've got a newer pylint, it'll whine about positional arguments
separately from the regular ones. Update the configuration to ignore
both categories of warning.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/pylintrc | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc
index c028a1f9f51..d24eece7411 100644
--- a/scripts/qapi/pylintrc
+++ b/scripts/qapi/pylintrc
@@ -17,6 +17,7 @@ disable=consider-using-f-string,
         too-many-arguments,
         too-many-branches,
         too-many-instance-attributes,
+        too-many-positional-arguments,
         too-many-statements,
         useless-option-value,
 
-- 
2.48.1

Re: [PATCH 01/10] qapi: update pylintrc config

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> If you've got a newer pylint, it'll whine about positional arguments
> separately from the regular ones. Update the configuration to ignore
> both categories of warning.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/pylintrc | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc
> index c028a1f9f51..d24eece7411 100644
> --- a/scripts/qapi/pylintrc
> +++ b/scripts/qapi/pylintrc
> @@ -17,6 +17,7 @@ disable=consider-using-f-string,
>          too-many-arguments,
>          too-many-branches,
>          too-many-instance-attributes,
> +        too-many-positional-arguments,
>          too-many-statements,
>          useless-option-value,

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[PATCH 02/10] python: add qapi static analysis tests

[John Snow]

Update the python tests to also check qapi. No idea why I didn't do this
before. I guess I was counting on moving it under python/ and then just
forgot after that was NACKed. Oops, this turns out to be really easy.

flake8, isort and mypy use the tool configuration from the existing
python directory. pylint continues to use the special configuration
located in scripts/qapi/ - that configuration is more permissive. If we
wish to unify the two configurations, that's a separate series and a
discussion for a later date.

As a result of this patch, one would be able to run any of the following
tests locally from the qemu.git/python directory and have it cover the
scripts/qapi/ module as well. All of the following options run the
python tests, static analysis tests, and linter checks; but with
different combinations of dependencies and interpreters.

- "make check-minreqs" Run tests specifically under our oldest supported
  Python and our oldest supported dependencies. This is the test that
  runs on GitLab as "check-python-minreqs". This helps ensure we do not
  regress support on older platforms accidentally.

- "make check-tox" Runs the tests under the newest supported
  dependencies, but under each supported version of Python in turn. At
  time of writing, this is Python 3.8 to 3.13 inclusive. This test helps
  catch bleeding-edge problems before they become problems for developer
  workstations. This is the GitLab test "check-python-tox" and is an
  optionally run, may-fail test due to the unpredictable nature of new
  dependencies being released into the ecosystem that may cause
  regressions.

- "make check-dev" Runs the tests under the newest supported
  dependencies using whatever version of Python the user happens to have
  installed. This is a quick convenience check that does not map to any
  particular GitLab test.

(Note! check-dev may be busted on Fedora 41 and bleeding edge versions
of setuptools. That's unrelated to this patch and I'll address it
separately and soon. Thank you for your patience, --mgmt)

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/tests/qapi-flake8.sh | 2 ++
 python/tests/qapi-isort.sh  | 2 ++
 python/tests/qapi-mypy.sh   | 2 ++
 python/tests/qapi-pylint.sh | 4 ++++
 4 files changed, 10 insertions(+)
 create mode 100755 python/tests/qapi-flake8.sh
 create mode 100755 python/tests/qapi-isort.sh
 create mode 100755 python/tests/qapi-mypy.sh
 create mode 100755 python/tests/qapi-pylint.sh

diff --git a/python/tests/qapi-flake8.sh b/python/tests/qapi-flake8.sh
new file mode 100755
index 00000000000..51916a9019d
--- /dev/null
+++ b/python/tests/qapi-flake8.sh
@@ -0,0 +1,2 @@
+#!/bin/sh -e
+python3 -m flake8 ../scripts/qapi/
diff --git a/python/tests/qapi-isort.sh b/python/tests/qapi-isort.sh
new file mode 100755
index 00000000000..60ed5eeb34d
--- /dev/null
+++ b/python/tests/qapi-isort.sh
@@ -0,0 +1,2 @@
+#!/bin/sh -e
+python3 -m isort --sp . -c ../scripts/qapi/
diff --git a/python/tests/qapi-mypy.sh b/python/tests/qapi-mypy.sh
new file mode 100755
index 00000000000..377b29b873b
--- /dev/null
+++ b/python/tests/qapi-mypy.sh
@@ -0,0 +1,2 @@
+#!/bin/sh -e
+python3 -m mypy ../scripts/qapi
diff --git a/python/tests/qapi-pylint.sh b/python/tests/qapi-pylint.sh
new file mode 100755
index 00000000000..d4869578e54
--- /dev/null
+++ b/python/tests/qapi-pylint.sh
@@ -0,0 +1,4 @@
+#!/bin/sh -e
+SETUPTOOLS_USE_DISTUTILS=stdlib python3 -m pylint \
+                                --rcfile=../scripts/qapi/pylintrc \
+                                ../scripts/qapi/
-- 
2.48.1

Re: [PATCH 02/10] python: add qapi static analysis tests

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Update the python tests to also check qapi. No idea why I didn't do this
> before. I guess I was counting on moving it under python/ and then just
> forgot after that was NACKed. Oops, this turns out to be really easy.
>
> flake8, isort and mypy use the tool configuration from the existing
> python directory. pylint continues to use the special configuration
> located in scripts/qapi/ - that configuration is more permissive. If we
> wish to unify the two configurations, that's a separate series and a
> discussion for a later date.
>
> As a result of this patch, one would be able to run any of the following
> tests locally from the qemu.git/python directory and have it cover the
> scripts/qapi/ module as well. All of the following options run the
> python tests, static analysis tests, and linter checks; but with
> different combinations of dependencies and interpreters.
>
> - "make check-minreqs" Run tests specifically under our oldest supported
>   Python and our oldest supported dependencies. This is the test that
>   runs on GitLab as "check-python-minreqs". This helps ensure we do not
>   regress support on older platforms accidentally.
>
> - "make check-tox" Runs the tests under the newest supported
>   dependencies, but under each supported version of Python in turn. At
>   time of writing, this is Python 3.8 to 3.13 inclusive. This test helps
>   catch bleeding-edge problems before they become problems for developer
>   workstations. This is the GitLab test "check-python-tox" and is an
>   optionally run, may-fail test due to the unpredictable nature of new
>   dependencies being released into the ecosystem that may cause
>   regressions.
>
> - "make check-dev" Runs the tests under the newest supported
>   dependencies using whatever version of Python the user happens to have
>   installed. This is a quick convenience check that does not map to any
>   particular GitLab test.
>
> (Note! check-dev may be busted on Fedora 41 and bleeding edge versions

It is for me.

> of setuptools. That's unrelated to this patch and I'll address it
> separately and soon. Thank you for your patience, --mgmt)

Which of these tests, if any, run in "make check"?  In CI?

> Signed-off-by: John Snow <jsnow@redhat.com>

Re: [PATCH 02/10] python: add qapi static analysis tests

[John Snow]

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

On Mon, Feb 24, 2025 at 7:36 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > Update the python tests to also check qapi. No idea why I didn't do this
> > before. I guess I was counting on moving it under python/ and then just
> > forgot after that was NACKed. Oops, this turns out to be really easy.
> >
> > flake8, isort and mypy use the tool configuration from the existing
> > python directory. pylint continues to use the special configuration
> > located in scripts/qapi/ - that configuration is more permissive. If we
> > wish to unify the two configurations, that's a separate series and a
> > discussion for a later date.
> >
> > As a result of this patch, one would be able to run any of the following
> > tests locally from the qemu.git/python directory and have it cover the
> > scripts/qapi/ module as well. All of the following options run the
> > python tests, static analysis tests, and linter checks; but with
> > different combinations of dependencies and interpreters.
> >
> > - "make check-minreqs" Run tests specifically under our oldest supported
> >   Python and our oldest supported dependencies. This is the test that
> >   runs on GitLab as "check-python-minreqs". This helps ensure we do not
> >   regress support on older platforms accidentally.
> >
> > - "make check-tox" Runs the tests under the newest supported
> >   dependencies, but under each supported version of Python in turn. At
> >   time of writing, this is Python 3.8 to 3.13 inclusive. This test helps
> >   catch bleeding-edge problems before they become problems for developer
> >   workstations. This is the GitLab test "check-python-tox" and is an
> >   optionally run, may-fail test due to the unpredictable nature of new
> >   dependencies being released into the ecosystem that may cause
> >   regressions.
> >
> > - "make check-dev" Runs the tests under the newest supported
> >   dependencies using whatever version of Python the user happens to have
> >   installed. This is a quick convenience check that does not map to any
> >   particular GitLab test.
> >
> > (Note! check-dev may be busted on Fedora 41 and bleeding edge versions
>
> It is for me.
>
> > of setuptools. That's unrelated to this patch and I'll address it
> > separately and soon. Thank you for your patience, --mgmt)
>
> Which of these tests, if any, run in "make check"?  In CI?
>

Under "make check", the top-level test in qemu.git, none. I swear on my
future grave I am trying to fix that, but there are barriers to it. Adding
make check support means installing testing dependencies in the configure
venv, which means a slower ./configure invocation. I am trying to figure
out how to minimize this penalty for cases where we either do not want to,
or can't, run the python tests. It's a long story, we can talk about it
later.

In CI, the "check-minreqs" test will run by default as a must-pass test
under the job "check python minreqs".

"check-tox" is an optional job in the CI pipeline that is allowed to fail
as a warning, due to the nature of this test checking bleeding edge
dependencies.

All three local invocations run the exact same tests (literally "make
check" in the python dir), just under different combinations of
dependencies and python versions. "check-minreqs" is more or less the
"canonical" one that *must* succeed, but as a Python maintainer I do my
best to enforce "check-tox" as well, though it does lag behind.

So, this isn't a perfect solution yet but it's certainly much better than
carrying around ad-hoc linter shell scripts and attempting to manage the
dependencies yourself. At least we all have access to the same invocations.


>
> > Signed-off-by: John Snow <jsnow@redhat.com>
>
>

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

Re: [PATCH 02/10] python: add qapi static analysis tests

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Mon, Feb 24, 2025 at 7:36 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > Update the python tests to also check qapi. No idea why I didn't do this
>> > before. I guess I was counting on moving it under python/ and then just
>> > forgot after that was NACKed. Oops, this turns out to be really easy.
>> >
>> > flake8, isort and mypy use the tool configuration from the existing
>> > python directory. pylint continues to use the special configuration
>> > located in scripts/qapi/ - that configuration is more permissive. If we
>> > wish to unify the two configurations, that's a separate series and a
>> > discussion for a later date.
>> >
>> > As a result of this patch, one would be able to run any of the following
>> > tests locally from the qemu.git/python directory and have it cover the
>> > scripts/qapi/ module as well. All of the following options run the
>> > python tests, static analysis tests, and linter checks; but with
>> > different combinations of dependencies and interpreters.
>> >
>> > - "make check-minreqs" Run tests specifically under our oldest supported
>> >   Python and our oldest supported dependencies. This is the test that
>> >   runs on GitLab as "check-python-minreqs". This helps ensure we do not
>> >   regress support on older platforms accidentally.
>> >
>> > - "make check-tox" Runs the tests under the newest supported
>> >   dependencies, but under each supported version of Python in turn. At
>> >   time of writing, this is Python 3.8 to 3.13 inclusive. This test helps
>> >   catch bleeding-edge problems before they become problems for developer
>> >   workstations. This is the GitLab test "check-python-tox" and is an
>> >   optionally run, may-fail test due to the unpredictable nature of new
>> >   dependencies being released into the ecosystem that may cause
>> >   regressions.
>> >
>> > - "make check-dev" Runs the tests under the newest supported
>> >   dependencies using whatever version of Python the user happens to have
>> >   installed. This is a quick convenience check that does not map to any
>> >   particular GitLab test.
>> >
>> > (Note! check-dev may be busted on Fedora 41 and bleeding edge versions
>>
>> It is for me.
>>
>> > of setuptools. That's unrelated to this patch and I'll address it
>> > separately and soon. Thank you for your patience, --mgmt)
>>
>> Which of these tests, if any, run in "make check"?  In CI?
>>
>
> Under "make check", the top-level test in qemu.git, none. I swear on my
> future grave

"Not today!"

>              I am trying to fix that,

Also not today.  SCNR!

>                                       but there are barriers to it. Adding
> make check support means installing testing dependencies in the configure
> venv, which means a slower ./configure invocation. I am trying to figure
> out how to minimize this penalty for cases where we either do not want to,
> or can't, run the python tests. It's a long story, we can talk about it
> later.
>
> In CI, the "check-minreqs" test will run by default as a must-pass test
> under the job "check python minreqs".
>
> "check-tox" is an optional job in the CI pipeline that is allowed to fail
> as a warning, due to the nature of this test checking bleeding edge
> dependencies.
>
> All three local invocations run the exact same tests (literally "make
> check" in the python dir), just under different combinations of
> dependencies and python versions. "check-minreqs" is more or less the
> "canonical" one that *must* succeed, but as a Python maintainer I do my
> best to enforce "check-tox" as well, though it does lag behind.
>
> So, this isn't a perfect solution yet but it's certainly much better than
> carrying around ad-hoc linter shell scripts and attempting to manage the
> dependencies yourself. At least we all have access to the same invocations.

So:

* At some point, we'll integrate whatever we want developers to run into
  "make check".  Until then:

* Running "make check-dev" is nice and good enough.  CI might find
  additional problems.  Expected to be rare and no big deal.

* Running "make check-minreqs" locally will get the exact same results
  as the same test in CI will.  Run if you care.

* "make check-tox" is an early warning system.  Don't run unless you're
  interested in preventing potential future problems.

Acked-by: Markus Armbruster <armbru@redhat.com>

[...]

Re: [PATCH 02/10] python: add qapi static analysis tests

[John Snow]

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

On Wed, Feb 26, 2025 at 4:29 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > On Mon, Feb 24, 2025 at 7:36 AM Markus Armbruster <armbru@redhat.com>
> wrote:
> >
> >> John Snow <jsnow@redhat.com> writes:
> >>
> >> > Update the python tests to also check qapi. No idea why I didn't do
> this
> >> > before. I guess I was counting on moving it under python/ and then
> just
> >> > forgot after that was NACKed. Oops, this turns out to be really easy.
> >> >
> >> > flake8, isort and mypy use the tool configuration from the existing
> >> > python directory. pylint continues to use the special configuration
> >> > located in scripts/qapi/ - that configuration is more permissive. If
> we
> >> > wish to unify the two configurations, that's a separate series and a
> >> > discussion for a later date.
> >> >
> >> > As a result of this patch, one would be able to run any of the
> following
> >> > tests locally from the qemu.git/python directory and have it cover the
> >> > scripts/qapi/ module as well. All of the following options run the
> >> > python tests, static analysis tests, and linter checks; but with
> >> > different combinations of dependencies and interpreters.
> >> >
> >> > - "make check-minreqs" Run tests specifically under our oldest
> supported
> >> >   Python and our oldest supported dependencies. This is the test that
> >> >   runs on GitLab as "check-python-minreqs". This helps ensure we do
> not
> >> >   regress support on older platforms accidentally.
> >> >
> >> > - "make check-tox" Runs the tests under the newest supported
> >> >   dependencies, but under each supported version of Python in turn. At
> >> >   time of writing, this is Python 3.8 to 3.13 inclusive. This test
> helps
> >> >   catch bleeding-edge problems before they become problems for
> developer
> >> >   workstations. This is the GitLab test "check-python-tox" and is an
> >> >   optionally run, may-fail test due to the unpredictable nature of new
> >> >   dependencies being released into the ecosystem that may cause
> >> >   regressions.
> >> >
> >> > - "make check-dev" Runs the tests under the newest supported
> >> >   dependencies using whatever version of Python the user happens to
> have
> >> >   installed. This is a quick convenience check that does not map to
> any
> >> >   particular GitLab test.
> >> >
> >> > (Note! check-dev may be busted on Fedora 41 and bleeding edge versions
> >>
> >> It is for me.
> >>
> >> > of setuptools. That's unrelated to this patch and I'll address it
> >> > separately and soon. Thank you for your patience, --mgmt)
> >>
> >> Which of these tests, if any, run in "make check"?  In CI?
> >>
> >
> > Under "make check", the top-level test in qemu.git, none. I swear on my
> > future grave
>
> "Not today!"
>
> >              I am trying to fix that,
>
> Also not today.  SCNR!
>
> >                                       but there are barriers to it.
> Adding
> > make check support means installing testing dependencies in the configure
> > venv, which means a slower ./configure invocation. I am trying to figure
> > out how to minimize this penalty for cases where we either do not want
> to,
> > or can't, run the python tests. It's a long story, we can talk about it
> > later.
> >
> > In CI, the "check-minreqs" test will run by default as a must-pass test
> > under the job "check python minreqs".
> >
> > "check-tox" is an optional job in the CI pipeline that is allowed to fail
> > as a warning, due to the nature of this test checking bleeding edge
> > dependencies.
> >
> > All three local invocations run the exact same tests (literally "make
> > check" in the python dir), just under different combinations of
> > dependencies and python versions. "check-minreqs" is more or less the
> > "canonical" one that *must* succeed, but as a Python maintainer I do my
> > best to enforce "check-tox" as well, though it does lag behind.
> >
> > So, this isn't a perfect solution yet but it's certainly much better than
> > carrying around ad-hoc linter shell scripts and attempting to manage the
> > dependencies yourself. At least we all have access to the same
> invocations.
>
> So:
>
> * At some point, we'll integrate whatever we want developers to run into
>   "make check".  Until then:
>
> * Running "make check-dev" is nice and good enough.  CI might find
>   additional problems.  Expected to be rare and no big deal.
>
> * Running "make check-minreqs" locally will get the exact same results
>   as the same test in CI will.  Run if you care.
>
> * "make check-tox" is an early warning system.  Don't run unless you're
>   interested in preventing potential future problems.
>

More or less; though it does test in every supported python interpreter if
you happen to have multiple installed, so it can be a way to catch errors
that exist between minreqs and $current, but it's still generally only a
test that I think you should run if you are touching the Python stuff in a
major way; i.e. if you're sending something that's a PR for *me*, I think
you should run it. If you're just doing a quick fix to qapi that doesn't do
any deep Python trickery, I'd trust either check-minreqs/CI or check-dev to
be sufficient due diligence.

In other words: *any one* of these tests are likely to catch errors due to
incorrect code and is sufficient due diligence; making sure they *all* pass
is more of a job for *me* to catch ecosystem problems across our wide
platform and python version support matrix.

I very often run "check minreqs" and "check tox" and consider that
exhaustive. the dev test is there only as a quick smoke test if you don't
have a battleship of python interpreters installed, but it's busted at the
moment due to fairly recent setuptools changes @_@

--js


>
> Acked-by: Markus Armbruster <armbru@redhat.com>
>
> [...]
>
>

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

[PATCH 03/10] qapi: delete un-needed python static analysis configs

[John Snow]

The pylint config is being left in place because the settings differ
enough from the python/ directory settings that we need a chit-chat on
how to merge them O:-)

Everything else can go.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/.flake8    | 3 ---
 scripts/qapi/.isort.cfg | 7 -------
 scripts/qapi/mypy.ini   | 4 ----
 3 files changed, 14 deletions(-)
 delete mode 100644 scripts/qapi/.flake8
 delete mode 100644 scripts/qapi/.isort.cfg
 delete mode 100644 scripts/qapi/mypy.ini

diff --git a/scripts/qapi/.flake8 b/scripts/qapi/.flake8
deleted file mode 100644
index a873ff67309..00000000000
--- a/scripts/qapi/.flake8
+++ /dev/null
@@ -1,3 +0,0 @@
-[flake8]
-# Prefer pylint's bare-except checks to flake8's
-extend-ignore = E722
diff --git a/scripts/qapi/.isort.cfg b/scripts/qapi/.isort.cfg
deleted file mode 100644
index 643caa1fbd6..00000000000
--- a/scripts/qapi/.isort.cfg
+++ /dev/null
@@ -1,7 +0,0 @@
-[settings]
-force_grid_wrap=4
-force_sort_within_sections=True
-include_trailing_comma=True
-line_length=72
-lines_after_imports=2
-multi_line_output=3
diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
deleted file mode 100644
index 8109470a031..00000000000
--- a/scripts/qapi/mypy.ini
+++ /dev/null
@@ -1,4 +0,0 @@
-[mypy]
-strict = True
-disallow_untyped_calls = False
-python_version = 3.8
-- 
2.48.1

Re: [PATCH 03/10] qapi: delete un-needed python static analysis configs

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> The pylint config is being left in place because the settings differ
> enough from the python/ directory settings that we need a chit-chat on
> how to merge them O:-)
>
> Everything else can go.
>
> Signed-off-by: John Snow <jsnow@redhat.com>

I tried to compare the contents of the deleted files to "the python/
directory settings", but I can't find the latter.  Am I confused?

Re: [PATCH 03/10] qapi: delete un-needed python static analysis configs

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> John Snow <jsnow@redhat.com> writes:
>
>> The pylint config is being left in place because the settings differ
>> enough from the python/ directory settings that we need a chit-chat on
>> how to merge them O:-)
>>
>> Everything else can go.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>
> I tried to compare the contents of the deleted files to "the python/
> directory settings", but I can't find the latter.  Am I confused?

John told me: it's in python/setup.cfg.

Re: [PATCH 03/10] qapi: delete un-needed python static analysis configs

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> The pylint config is being left in place because the settings differ
> enough from the python/ directory settings that we need a chit-chat on
> how to merge them O:-)
>
> Everything else can go.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/.flake8    | 3 ---
>  scripts/qapi/.isort.cfg | 7 -------
>  scripts/qapi/mypy.ini   | 4 ----
>  3 files changed, 14 deletions(-)
>  delete mode 100644 scripts/qapi/.flake8
>  delete mode 100644 scripts/qapi/.isort.cfg
>  delete mode 100644 scripts/qapi/mypy.ini
>
> diff --git a/scripts/qapi/.flake8 b/scripts/qapi/.flake8
> deleted file mode 100644
> index a873ff67309..00000000000
> --- a/scripts/qapi/.flake8
> +++ /dev/null
> @@ -1,3 +0,0 @@
> -[flake8]
> -# Prefer pylint's bare-except checks to flake8's
> -extend-ignore = E722

python/setup.cfg has:

   [flake8]
   # Prefer pylint's bare-except checks to flake8's
   extend-ignore = E722
   exclude = __pycache__,

Good.

> diff --git a/scripts/qapi/.isort.cfg b/scripts/qapi/.isort.cfg
> deleted file mode 100644
> index 643caa1fbd6..00000000000
> --- a/scripts/qapi/.isort.cfg
> +++ /dev/null
> @@ -1,7 +0,0 @@
> -[settings]
> -force_grid_wrap=4
> -force_sort_within_sections=True
> -include_trailing_comma=True
> -line_length=72
> -lines_after_imports=2
> -multi_line_output=3

python/setup.cfg has:

   [isort]
   force_grid_wrap=4
   force_sort_within_sections=True
   include_trailing_comma=True
   line_length=72
   lines_after_imports=2
   multi_line_output=3

Good.

> diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
> deleted file mode 100644
> index 8109470a031..00000000000
> --- a/scripts/qapi/mypy.ini
> +++ /dev/null
> @@ -1,4 +0,0 @@
> -[mypy]
> -strict = True
> -disallow_untyped_calls = False
> -python_version = 3.8

python/setup.cfg has:

   [mypy]
   strict = True
   python_version = 3.8
   warn_unused_configs = True
   namespace_packages = True
   warn_unused_ignores = False

Can you briefly explain the differences?

python/setup.cfg additionally has a bunch of ignore_missing_imports that
don't apply here, as far as I can tell.

Re: [PATCH 03/10] qapi: delete un-needed python static analysis configs

[John Snow]

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

On Wed, Feb 26, 2025 at 2:28 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > The pylint config is being left in place because the settings differ
> > enough from the python/ directory settings that we need a chit-chat on
> > how to merge them O:-)
> >
> > Everything else can go.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  scripts/qapi/.flake8    | 3 ---
> >  scripts/qapi/.isort.cfg | 7 -------
> >  scripts/qapi/mypy.ini   | 4 ----
> >  3 files changed, 14 deletions(-)
> >  delete mode 100644 scripts/qapi/.flake8
> >  delete mode 100644 scripts/qapi/.isort.cfg
> >  delete mode 100644 scripts/qapi/mypy.ini
> >
> > diff --git a/scripts/qapi/.flake8 b/scripts/qapi/.flake8
> > deleted file mode 100644
> > index a873ff67309..00000000000
> > --- a/scripts/qapi/.flake8
> > +++ /dev/null
> > @@ -1,3 +0,0 @@
> > -[flake8]
> > -# Prefer pylint's bare-except checks to flake8's
> > -extend-ignore = E722
>
> python/setup.cfg has:
>
>    [flake8]
>    # Prefer pylint's bare-except checks to flake8's
>    extend-ignore = E722
>    exclude = __pycache__,
>
> Good.
>
> > diff --git a/scripts/qapi/.isort.cfg b/scripts/qapi/.isort.cfg
> > deleted file mode 100644
> > index 643caa1fbd6..00000000000
> > --- a/scripts/qapi/.isort.cfg
> > +++ /dev/null
> > @@ -1,7 +0,0 @@
> > -[settings]
> > -force_grid_wrap=4
> > -force_sort_within_sections=True
> > -include_trailing_comma=True
> > -line_length=72
> > -lines_after_imports=2
> > -multi_line_output=3
>
> python/setup.cfg has:
>
>    [isort]
>    force_grid_wrap=4
>    force_sort_within_sections=True
>    include_trailing_comma=True
>    line_length=72
>    lines_after_imports=2
>    multi_line_output=3
>
> Good.
>
> > diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
> > deleted file mode 100644
> > index 8109470a031..00000000000
> > --- a/scripts/qapi/mypy.ini
> > +++ /dev/null
> > @@ -1,4 +0,0 @@
> > -[mypy]
> > -strict = True
> > -disallow_untyped_calls = False
> > -python_version = 3.8
>
> python/setup.cfg has:
>
>    [mypy]
>    strict = True
>    python_version = 3.8
>    warn_unused_configs = True
>    namespace_packages = True
>    warn_unused_ignores = False
>
> Can you briefly explain the differences?
>

warn_unused_configs: Catches config values that aren't actually recognized
or used. Was helpful once upon a time when re-arranging the Python
directory to behave like a package to ensure that the conf files were
working correctly.

namespace_packages: Needed for the python/ directory structure (nested
packages under a namespace, "qemu"). Doesn't impact scripts/qapi at all.
Read up on PEP420 if you are curious. Details in commit message, see below
if you're still curious.

warn_unused_ignores: Needed once upon a time for cross-version mypy support
where some versions would warn in some cases and others would not. Adding
an ignore would effectively just invert which versions complained. Probably
still needed, but it's hard to measure.

python_version: Changes mypy behavior regardless of the invoking python
interpreter to check the file as if it were to be executed by Python 3.8. I
actually want to remove this value from setup.cfg but haven't yet. I
removed it from the python-qemu-qmp repo and never added it for qapi.
Removing it is actually probably correct as it will catch errors specific
to various python versions we support, but there are some nits to iron out
in my neck of the woods. This is a case where scripts/qapi/ is stricter
than python/ :)
(Not reasonable to solve for this series.)

lack of disallow_untyped_calls = False: I think this might be a remnant
from when we gradually typed qapi; it's evidently no longer needed since
qapi still checks fine without this affordance. The default under strict is
True.

e941c844e444 (John Snow                   2021-05-27 17:17:05 -0400  79)
[mypy]
e941c844e444 (John Snow                   2021-05-27 17:17:05 -0400  80)
strict = True
ca056f4499c2 (Paolo Bonzini               2023-05-03 12:48:02 +0200  81)
python_version = 3.8
e941c844e444 (John Snow                   2021-05-27 17:17:05 -0400  82)
warn_unused_configs = True
0542a4c95767 (John Snow                   2021-05-27 17:17:06 -0400  83)
namespace_packages = True
e7874a50ff3f (John Snow                   2022-05-25 20:09:13 -0400  84)
warn_unused_ignores = False


>
> python/setup.cfg additionally has a bunch of ignore_missing_imports that
> don't apply here, as far as I can tell.
>

Right, that's all stuff for fuse and the interactive qmp shell that use
untyped dependencies.

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

Re: [PATCH 03/10] qapi: delete un-needed python static analysis configs

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Wed, Feb 26, 2025 at 2:28 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > The pylint config is being left in place because the settings differ
>> > enough from the python/ directory settings that we need a chit-chat on
>> > how to merge them O:-)
>> >
>> > Everything else can go.
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>

[...]

>> > diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
>> > deleted file mode 100644
>> > index 8109470a031..00000000000
>> > --- a/scripts/qapi/mypy.ini
>> > +++ /dev/null
>> > @@ -1,4 +0,0 @@
>> > -[mypy]
>> > -strict = True
>> > -disallow_untyped_calls = False
>> > -python_version = 3.8
>>
>> python/setup.cfg has:
>>
>>    [mypy]
>>    strict = True
>>    python_version = 3.8
>>    warn_unused_configs = True
>>    namespace_packages = True
>>    warn_unused_ignores = False
>>
>> Can you briefly explain the differences?
>>
>
> warn_unused_configs: Catches config values that aren't actually recognized
> or used. Was helpful once upon a time when re-arranging the Python
> directory to behave like a package to ensure that the conf files were
> working correctly.

Could this be culled now?

Hmm, according to mypy(1), strict implies warn-unused-configs.

The question does not block this patch.

> namespace_packages: Needed for the python/ directory structure (nested
> packages under a namespace, "qemu"). Doesn't impact scripts/qapi at all.
> Read up on PEP420 if you are curious. Details in commit message, see below
> if you're still curious.

mypy(1) makes me suspect this is the default.  If that's true across the
versions we care for, this could be culled.

Also does not block this patch.

> warn_unused_ignores: Needed once upon a time for cross-version mypy support
> where some versions would warn in some cases and others would not. Adding
> an ignore would effectively just invert which versions complained. Probably
> still needed, but it's hard to measure.

Harmless enough.

> python_version: Changes mypy behavior regardless of the invoking python
> interpreter to check the file as if it were to be executed by Python 3.8. I
> actually want to remove this value from setup.cfg but haven't yet. I
> removed it from the python-qemu-qmp repo and never added it for qapi.
> Removing it is actually probably correct as it will catch errors specific
> to various python versions we support, but there are some nits to iron out
> in my neck of the woods. This is a case where scripts/qapi/ is stricter
> than python/ :)
> (Not reasonable to solve for this series.)

Also present in the deleted file, so no change.

> lack of disallow_untyped_calls = False: I think this might be a remnant
> from when we gradually typed qapi; it's evidently no longer needed since
> qapi still checks fine without this affordance. The default under strict is
> True.

Fair enough.

> e941c844e444 (John Snow                   2021-05-27 17:17:05 -0400  79)
> [mypy]
> e941c844e444 (John Snow                   2021-05-27 17:17:05 -0400  80)
> strict = True
> ca056f4499c2 (Paolo Bonzini               2023-05-03 12:48:02 +0200  81)
> python_version = 3.8
> e941c844e444 (John Snow                   2021-05-27 17:17:05 -0400  82)
> warn_unused_configs = True
> 0542a4c95767 (John Snow                   2021-05-27 17:17:06 -0400  83)
> namespace_packages = True
> e7874a50ff3f (John Snow                   2022-05-25 20:09:13 -0400  84)
> warn_unused_ignores = False
>
>
>>
>> python/setup.cfg additionally has a bunch of ignore_missing_imports that
>> don't apply here, as far as I can tell.
>>
>
> Right, that's all stuff for fuse and the interactive qmp shell that use
> untyped dependencies.

Good.

Let's mention the differences in the commit message.  Here's my try:

    Since the previous commit, python/setup.cfg applies to scripts/qapi/
    as well.  Configuration files in scripts/qapi/ override
    python/setup.cfg.

    scripts/qapi/.flake8 and scripts/qapi/.isort.cfg actually match
    python/setup.cfg exactly, and can go.

    The differences between scripts/qapi/mypy.ini and python/setup.cfg
    are harmless: [list the differences, explain why they're harmless as
    long as you can keep it brief, and if not, fall back to "trust me"].
    So scripts/qapi/mypy.ini can go, too.

    The pylint config is being left in place because the settings differ
    enough from the python/ directory settings that we need a chit-chat on
    how to merge them O:-)

With something like that
Reviewed-by: Markus Armbruster <armbru@redhat.com>

[PATCH 04/10] docs/qapidoc: support header-less freeform sections

[John Snow]

The code as written crashes when a free-form documentation block doesn't
start with a heading or subheading, for example:

| ##
| # Just text, no heading.
| ##

The code will attempt to use the `node` variable uninitialized. To fix,
create a generic block to insert the doc text into.

(This patch also removes a lingering pylint warning in the QAPIDoc
implementation that prevents getting a clean baseline to use for
forthcoming additions.)

Fixes: 43e0d14ee09a (docs/sphinx: fix extra stuff in TOC after freeform QMP sections)
Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/sphinx/qapidoc.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 5f96b46270b..5a4d7388b29 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -421,6 +421,8 @@ def freeform(self, doc):
             node = self._start_new_heading(heading, len(leader))
             if text == '':
                 return
+        else:
+            node = nodes.container()
 
         self._parse_text_into_node(text, node)
         self._cur_doc = None
-- 
2.48.1

Re: [PATCH 04/10] docs/qapidoc: support header-less freeform sections

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> The code as written crashes when a free-form documentation block doesn't
> start with a heading or subheading, for example:
>
> | ##
> | # Just text, no heading.
> | ##
>
> The code will attempt to use the `node` variable uninitialized. To fix,
> create a generic block to insert the doc text into.
>
> (This patch also removes a lingering pylint warning in the QAPIDoc
> implementation that prevents getting a clean baseline to use for
> forthcoming additions.)
>
> Fixes: 43e0d14ee09a (docs/sphinx: fix extra stuff in TOC after freeform QMP sections)
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  docs/sphinx/qapidoc.py | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index 5f96b46270b..5a4d7388b29 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -421,6 +421,8 @@ def freeform(self, doc):
>              node = self._start_new_heading(heading, len(leader))
>              if text == '':
>                  return
> +        else:
> +            node = nodes.container()
>  
>          self._parse_text_into_node(text, node)
>          self._cur_doc = None

Let's add a suitable free-form block to tests/qapi-schema/doc-good.json
to catch regressions.  Not worth a respin by itself.

Re: [PATCH 04/10] docs/qapidoc: support header-less freeform sections

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> John Snow <jsnow@redhat.com> writes:
>
>> The code as written crashes when a free-form documentation block doesn't
>> start with a heading or subheading, for example:
>>
>> | ##
>> | # Just text, no heading.
>> | ##
>>
>> The code will attempt to use the `node` variable uninitialized. To fix,
>> create a generic block to insert the doc text into.
>>
>> (This patch also removes a lingering pylint warning in the QAPIDoc
>> implementation that prevents getting a clean baseline to use for
>> forthcoming additions.)
>>
>> Fixes: 43e0d14ee09a (docs/sphinx: fix extra stuff in TOC after freeform QMP sections)
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  docs/sphinx/qapidoc.py | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
>> index 5f96b46270b..5a4d7388b29 100644
>> --- a/docs/sphinx/qapidoc.py
>> +++ b/docs/sphinx/qapidoc.py
>> @@ -421,6 +421,8 @@ def freeform(self, doc):
>>              node = self._start_new_heading(heading, len(leader))
>>              if text == '':
>>                  return
>> +        else:
>> +            node = nodes.container()
>>  
>>          self._parse_text_into_node(text, node)
>>          self._cur_doc = None
>
> Let's add a suitable free-form block to tests/qapi-schema/doc-good.json
> to catch regressions.  Not worth a respin by itself.

With the appended obvious fixup:
Reviewed-by: Markus Armbruster <armbru@redhat.com>


diff --git a/tests/qapi-schema/doc-good.json b/tests/qapi-schema/doc-good.json
index f64bf38d85..0a4f139f83 100644
--- a/tests/qapi-schema/doc-good.json
+++ b/tests/qapi-schema/doc-good.json
@@ -11,6 +11,10 @@
 # = Section
 ##
 
+##
+# Just text, no heading.
+##
+
 ##
 # == Subsection
 #
diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
index ec277be91e..0a9da3efde 100644
--- a/tests/qapi-schema/doc-good.out
+++ b/tests/qapi-schema/doc-good.out
@@ -56,6 +56,9 @@ event EVT_BOXED Object
 doc freeform
     body=
 = Section
+doc freeform
+    body=
+Just text, no heading.
 doc freeform
     body=
 == Subsection

Re: [PATCH 04/10] docs/qapidoc: support header-less freeform sections

[John Snow]

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

On Wed, Feb 26, 2025, 4:37 AM Markus Armbruster <armbru@redhat.com> wrote:

> Markus Armbruster <armbru@redhat.com> writes:
>
> > John Snow <jsnow@redhat.com> writes:
> >
> >> The code as written crashes when a free-form documentation block doesn't
> >> start with a heading or subheading, for example:
> >>
> >> | ##
> >> | # Just text, no heading.
> >> | ##
> >>
> >> The code will attempt to use the `node` variable uninitialized. To fix,
> >> create a generic block to insert the doc text into.
> >>
> >> (This patch also removes a lingering pylint warning in the QAPIDoc
> >> implementation that prevents getting a clean baseline to use for
> >> forthcoming additions.)
> >>
> >> Fixes: 43e0d14ee09a (docs/sphinx: fix extra stuff in TOC after freeform
> QMP sections)
> >> Signed-off-by: John Snow <jsnow@redhat.com>
> >> ---
> >>  docs/sphinx/qapidoc.py | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> >> index 5f96b46270b..5a4d7388b29 100644
> >> --- a/docs/sphinx/qapidoc.py
> >> +++ b/docs/sphinx/qapidoc.py
> >> @@ -421,6 +421,8 @@ def freeform(self, doc):
> >>              node = self._start_new_heading(heading, len(leader))
> >>              if text == '':
> >>                  return
> >> +        else:
> >> +            node = nodes.container()
> >>
> >>          self._parse_text_into_node(text, node)
> >>          self._cur_doc = None
> >
> > Let's add a suitable free-form block to tests/qapi-schema/doc-good.json
> > to catch regressions.  Not worth a respin by itself.
>
> With the appended obvious fixup:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
>
> diff --git a/tests/qapi-schema/doc-good.json
> b/tests/qapi-schema/doc-good.json
> index f64bf38d85..0a4f139f83 100644
> --- a/tests/qapi-schema/doc-good.json
> +++ b/tests/qapi-schema/doc-good.json
> @@ -11,6 +11,10 @@
>  # = Section
>  ##
>
> +##
> +# Just text, no heading.
> +##
> +
>  ##
>  # == Subsection
>  #
> diff --git a/tests/qapi-schema/doc-good.out
> b/tests/qapi-schema/doc-good.out
> index ec277be91e..0a9da3efde 100644
> --- a/tests/qapi-schema/doc-good.out
> +++ b/tests/qapi-schema/doc-good.out
> @@ -56,6 +56,9 @@ event EVT_BOXED Object
>  doc freeform
>      body=
>  = Section
> +doc freeform
> +    body=
> +Just text, no heading.
>  doc freeform
>      body=
>  == Subsection
>

Sold!

>

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

[PATCH 05/10] qapi/parser: adjust info location for doc body section

[John Snow]

Instead of using the info object for the doc block as a whole (which
always points to the very first line of the block), update the info
pointer for each call to ensure_untagged_section when the existing
section is otherwise empty. This way, Sphinx error information will
match precisely to where the text actually starts.

For example, this patch will move the info pointer for the "Hello!"
untagged section ...

> ##       <-- from here ...
> # Hello! <-- ... to here.
> ##

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/parser.py | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index adc85b5b394..36cb64a677a 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -687,7 +687,11 @@ def end(self) -> None:
     def ensure_untagged_section(self, info: QAPISourceInfo) -> None:
         if self.all_sections and not self.all_sections[-1].tag:
             # extend current section
-            self.all_sections[-1].text += '\n'
+            section = self.all_sections[-1]
+            if not section.text:
+                # Section is empty so far; update info to start *here*.
+                section.info = info
+            section.text += '\n'
             return
         # start new section
         section = self.Section(info)
-- 
2.48.1

Re: [PATCH 05/10] qapi/parser: adjust info location for doc body section

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Instead of using the info object for the doc block as a whole (which
> always points to the very first line of the block), update the info
> pointer for each call to ensure_untagged_section when the existing
> section is otherwise empty. This way, Sphinx error information will
> match precisely to where the text actually starts.
>
> For example, this patch will move the info pointer for the "Hello!"
> untagged section ...
>
>> ##       <-- from here ...
>> # Hello! <-- ... to here.
>> ##
>
> Signed-off-by: John Snow <jsnow@redhat.com>

This patch would be easier to accept with a test case where it improves
the error location.  I tried to construct one quickly, but failed.  Can
you help?

[PATCH 06/10] docs/qapidoc: remove example section support

[John Snow]

Since commit 3c5f6114 (qapi: remove "Example" doc section), Example
sections no longer exist, so this support in qapidoc is now dead code.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/sphinx/qapidoc.py | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 5a4d7388b29..61997fd21af 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -254,10 +254,6 @@ def _nodes_for_features(self, doc):
         section += dlnode
         return [section]
 
-    def _nodes_for_example(self, exampletext):
-        """Return list of doctree nodes for a code example snippet"""
-        return [nodes.literal_block(exampletext, exampletext)]
-
     def _nodes_for_sections(self, doc):
         """Return list of doctree nodes for additional sections"""
         nodelist = []
@@ -275,10 +271,7 @@ def _nodes_for_sections(self, doc):
                 continue
 
             snode = self._make_section(section.tag)
-            if section.tag.startswith('Example'):
-                snode += self._nodes_for_example(dedent(section.text))
-            else:
-                self._parse_text_into_node(dedent(section.text), snode)
+            self._parse_text_into_node(dedent(section.text), snode)
             nodelist.append(snode)
         return nodelist
 
-- 
2.48.1

Re: [PATCH 06/10] docs/qapidoc: remove example section support

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Since commit 3c5f6114 (qapi: remove "Example" doc section), Example
> sections no longer exist, so this support in qapidoc is now dead code.
>
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[PATCH 07/10] qapi: expand tags to all doc sections

[John Snow]

This patch adds an explicit section "kind" to all QAPIDoc
sections. Members/Features are now explicitly marked as such, with the
name now being stored in a dedicated "name" field (which qapidoc.py was
not actually using anyway.)

The qapi-schema tests are updated to account for the new section names;
mostly "TODO" becomes "Todo" and `None` becomes "Plain".

Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/sphinx/qapidoc.py         |   7 ++-
 scripts/qapi/parser.py         | 109 ++++++++++++++++++++++++---------
 tests/qapi-schema/doc-good.out |  10 +--
 tests/qapi-schema/test-qapi.py |   2 +-
 4 files changed, 90 insertions(+), 38 deletions(-)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 61997fd21af..d622398f1da 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -35,6 +35,7 @@
 from docutils.statemachine import ViewList
 from qapi.error import QAPIError, QAPISemError
 from qapi.gen import QAPISchemaVisitor
+from qapi.parser import QAPIDoc
 from qapi.schema import QAPISchema
 
 from sphinx import addnodes
@@ -258,11 +259,11 @@ def _nodes_for_sections(self, doc):
         """Return list of doctree nodes for additional sections"""
         nodelist = []
         for section in doc.sections:
-            if section.tag and section.tag == 'TODO':
+            if section.kind == QAPIDoc.Kind.TODO:
                 # Hide TODO: sections
                 continue
 
-            if not section.tag:
+            if section.kind == QAPIDoc.Kind.PLAIN:
                 # Sphinx cannot handle sectionless titles;
                 # Instead, just append the results to the prior section.
                 container = nodes.container()
@@ -270,7 +271,7 @@ def _nodes_for_sections(self, doc):
                 nodelist += container.children
                 continue
 
-            snode = self._make_section(section.tag)
+            snode = self._make_section(section.kind.name.title())
             self._parse_text_into_node(dedent(section.text), snode)
             nodelist.append(snode)
         return nodelist
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 36cb64a677a..c3004aa70c6 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -15,6 +15,7 @@
 # See the COPYING file in the top-level directory.
 
 from collections import OrderedDict
+import enum
 import os
 import re
 from typing import (
@@ -575,7 +576,10 @@ def get_doc(self) -> 'QAPIDoc':
                         )
                         raise QAPIParseError(self, emsg)
 
-                    doc.new_tagged_section(self.info, match.group(1))
+                    doc.new_tagged_section(
+                        self.info,
+                        QAPIDoc.Kind.from_string(match.group(1))
+                    )
                     text = line[match.end():]
                     if text:
                         doc.append_line(text)
@@ -586,7 +590,7 @@ def get_doc(self) -> 'QAPIDoc':
                         self,
                         "unexpected '=' markup in definition documentation")
                 else:
-                    # tag-less paragraph
+                    # plain paragraph(s)
                     doc.ensure_untagged_section(self.info)
                     doc.append_line(line)
                     line = self.get_doc_paragraph(doc)
@@ -635,14 +639,37 @@ class QAPIDoc:
     Free-form documentation blocks consist only of a body section.
     """
 
+    class Kind(enum.Enum):
+        PLAIN = 0
+        MEMBER = 1
+        FEATURE = 2
+        RETURNS = 3
+        ERRORS = 4
+        SINCE = 5
+        TODO = 6
+
+        @staticmethod
+        def from_string(kind: str) -> 'QAPIDoc.Kind':
+            return QAPIDoc.Kind[kind.upper()]
+
+        def text_required(self) -> bool:
+            # Only "plain" sections can be empty
+            return self.value not in (0,)
+
+        def __str__(self) -> str:
+            return self.name.title()
+
     class Section:
         # pylint: disable=too-few-public-methods
-        def __init__(self, info: QAPISourceInfo,
-                     tag: Optional[str] = None):
+        def __init__(
+            self,
+            info: QAPISourceInfo,
+            kind: 'QAPIDoc.Kind',
+        ):
             # section source info, i.e. where it begins
             self.info = info
-            # section tag, if any ('Returns', '@name', ...)
-            self.tag = tag
+            # section kind
+            self.kind = kind
             # section text without tag
             self.text = ''
 
@@ -650,8 +677,14 @@ def append_line(self, line: str) -> None:
             self.text += line + '\n'
 
     class ArgSection(Section):
-        def __init__(self, info: QAPISourceInfo, tag: str):
-            super().__init__(info, tag)
+        def __init__(
+            self,
+            info: QAPISourceInfo,
+            kind: 'QAPIDoc.Kind',
+            name: str
+        ):
+            super().__init__(info, kind)
+            self.name = name
             self.member: Optional['QAPISchemaMember'] = None
 
         def connect(self, member: 'QAPISchemaMember') -> None:
@@ -663,7 +696,9 @@ def __init__(self, info: QAPISourceInfo, symbol: Optional[str] = None):
         # definition doc's symbol, None for free-form doc
         self.symbol: Optional[str] = symbol
         # the sections in textual order
-        self.all_sections: List[QAPIDoc.Section] = [QAPIDoc.Section(info)]
+        self.all_sections: List[QAPIDoc.Section] = [
+            QAPIDoc.Section(info, QAPIDoc.Kind.PLAIN)
+        ]
         # the body section
         self.body: Optional[QAPIDoc.Section] = self.all_sections[0]
         # dicts mapping parameter/feature names to their description
@@ -680,12 +715,17 @@ def __init__(self, info: QAPISourceInfo, symbol: Optional[str] = None):
     def end(self) -> None:
         for section in self.all_sections:
             section.text = section.text.strip('\n')
-            if section.tag is not None and section.text == '':
+            if section.kind.text_required() and section.text == '':
                 raise QAPISemError(
-                    section.info, "text required after '%s:'" % section.tag)
+                    section.info, "text required after '%s:'" % section.kind)
 
-    def ensure_untagged_section(self, info: QAPISourceInfo) -> None:
-        if self.all_sections and not self.all_sections[-1].tag:
+    def ensure_untagged_section(
+        self,
+        info: QAPISourceInfo,
+    ) -> None:
+        kind = QAPIDoc.Kind.PLAIN
+
+        if self.all_sections and self.all_sections[-1].kind == kind:
             # extend current section
             section = self.all_sections[-1]
             if not section.text:
@@ -693,46 +733,56 @@ def ensure_untagged_section(self, info: QAPISourceInfo) -> None:
                 section.info = info
             section.text += '\n'
             return
+
         # start new section
-        section = self.Section(info)
+        section = self.Section(info, kind)
         self.sections.append(section)
         self.all_sections.append(section)
 
-    def new_tagged_section(self, info: QAPISourceInfo, tag: str) -> None:
-        section = self.Section(info, tag)
-        if tag == 'Returns':
+    def new_tagged_section(
+        self,
+        info: QAPISourceInfo,
+        kind: 'QAPIDoc.Kind',
+    ) -> None:
+        section = self.Section(info, kind)
+        if kind == QAPIDoc.Kind.RETURNS:
             if self.returns:
                 raise QAPISemError(
-                    info, "duplicated '%s' section" % tag)
+                    info, "duplicated '%s' section" % kind)
             self.returns = section
-        elif tag == 'Errors':
+        elif kind == QAPIDoc.Kind.ERRORS:
             if self.errors:
                 raise QAPISemError(
-                    info, "duplicated '%s' section" % tag)
+                    info, "duplicated '%s' section" % kind)
             self.errors = section
-        elif tag == 'Since':
+        elif kind == QAPIDoc.Kind.SINCE:
             if self.since:
                 raise QAPISemError(
-                    info, "duplicated '%s' section" % tag)
+                    info, "duplicated '%s' section" % kind)
             self.since = section
         self.sections.append(section)
         self.all_sections.append(section)
 
-    def _new_description(self, info: QAPISourceInfo, name: str,
-                         desc: Dict[str, ArgSection]) -> None:
+    def _new_description(
+        self,
+        info: QAPISourceInfo,
+        name: str,
+        kind: 'QAPIDoc.Kind',
+        desc: Dict[str, ArgSection]
+    ) -> None:
         if not name:
             raise QAPISemError(info, "invalid parameter name")
         if name in desc:
             raise QAPISemError(info, "'%s' parameter name duplicated" % name)
-        section = self.ArgSection(info, '@' + name)
+        section = self.ArgSection(info, kind, name)
         self.all_sections.append(section)
         desc[name] = section
 
     def new_argument(self, info: QAPISourceInfo, name: str) -> None:
-        self._new_description(info, name, self.args)
+        self._new_description(info, name, QAPIDoc.Kind.MEMBER, self.args)
 
     def new_feature(self, info: QAPISourceInfo, name: str) -> None:
-        self._new_description(info, name, self.features)
+        self._new_description(info, name, QAPIDoc.Kind.FEATURE, self.features)
 
     def append_line(self, line: str) -> None:
         self.all_sections[-1].append_line(line)
@@ -744,8 +794,9 @@ def connect_member(self, member: 'QAPISchemaMember') -> None:
                 raise QAPISemError(member.info,
                                    "%s '%s' lacks documentation"
                                    % (member.role, member.name))
-            self.args[member.name] = QAPIDoc.ArgSection(
-                self.info, '@' + member.name)
+            section = QAPIDoc.ArgSection(
+                self.info, QAPIDoc.Kind.MEMBER, member.name)
+            self.args[member.name] = section
         self.args[member.name].connect(member)
 
     def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
index ec277be91e9..2d33a305ee7 100644
--- a/tests/qapi-schema/doc-good.out
+++ b/tests/qapi-schema/doc-good.out
@@ -110,7 +110,7 @@ The _one_ {and only}, description on the same line
 Also _one_ {and only}
     feature=enum-member-feat
 a member feature
-    section=None
+    section=Plain
 @two is undocumented
 doc symbol=Base
     body=
@@ -168,15 +168,15 @@ description starts on the same line
 a feature
     feature=cmd-feat2
 another feature
-    section=None
+    section=Plain
 .. note:: @arg3 is undocumented
     section=Returns
 @Object
     section=Errors
 some
-    section=TODO
+    section=Todo
 frobnicate
-    section=None
+    section=Plain
 .. admonition:: Notes
 
  - Lorem ipsum dolor sit amet
@@ -209,7 +209,7 @@ If you're bored enough to read this, go see a video of boxed cats
 a feature
     feature=cmd-feat2
 another feature
-    section=None
+    section=Plain
 .. qmp-example::
 
    -> "this example"
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index 7e3f9f4aa1f..bca924309be 100755
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -131,7 +131,7 @@ def test_frontend(fname):
         for feat, section in doc.features.items():
             print('    feature=%s\n%s' % (feat, section.text))
         for section in doc.sections:
-            print('    section=%s\n%s' % (section.tag, section.text))
+            print('    section=%s\n%s' % (section.kind, section.text))
 
 
 def open_test_result(dir_name, file_name, update):
-- 
2.48.1

Re: [PATCH 07/10] qapi: expand tags to all doc sections

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> This patch adds an explicit section "kind" to all QAPIDoc
> sections. Members/Features are now explicitly marked as such, with the
> name now being stored in a dedicated "name" field (which qapidoc.py was
> not actually using anyway.)

I'm not sure what the parenthesis is trying to convey.

Before the patch, we have:

              type        tag
    untagged  Section     None
    @foo:     ArgSection  'foo'
    Returns:  Section     'Returns'
    Errors:   Section     'Errors'
    Since:    Section     'Since'
    TODO:     Section     'TODO'

Afterwards, I believe:

              type         kind     name
    untagged  Section      PLAIN
    @foo:     ArgSection   MEMBER   'foo'   if member or argument
              ArgSection   FEATURE  'foo'   if feature
    Returns:  Section      RETURNS
    Errors:   Section      ERRORS
    Since:    Section      SINCE
    TODO:     Section      TODO

So, .tag is replaced by .kind and .name, member vs. feature vs. other
tags is now obvious from .kind alone, i.e. there's no need to account
for context or type.

Fine print: why do we need to account for type before the patch?
Consider @Since: ...

> The qapi-schema tests are updated to account for the new section names;
> mostly "TODO" becomes "Todo" and `None` becomes "Plain".
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  docs/sphinx/qapidoc.py         |   7 ++-
>  scripts/qapi/parser.py         | 109 ++++++++++++++++++++++++---------
>  tests/qapi-schema/doc-good.out |  10 +--
>  tests/qapi-schema/test-qapi.py |   2 +-
>  4 files changed, 90 insertions(+), 38 deletions(-)
>
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index 61997fd21af..d622398f1da 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -35,6 +35,7 @@
>  from docutils.statemachine import ViewList
>  from qapi.error import QAPIError, QAPISemError
>  from qapi.gen import QAPISchemaVisitor
> +from qapi.parser import QAPIDoc
>  from qapi.schema import QAPISchema
>
>  from sphinx import addnodes
> @@ -258,11 +259,11 @@ def _nodes_for_sections(self, doc):
>          """Return list of doctree nodes for additional sections"""
>          nodelist = []
>          for section in doc.sections:
> -            if section.tag and section.tag == 'TODO':
> +            if section.kind == QAPIDoc.Kind.TODO:
>                  # Hide TODO: sections
>                  continue
>
> -            if not section.tag:
> +            if section.kind == QAPIDoc.Kind.PLAIN:
>                  # Sphinx cannot handle sectionless titles;
>                  # Instead, just append the results to the prior section.
>                  container = nodes.container()
> @@ -270,7 +271,7 @@ def _nodes_for_sections(self, doc):
>                  nodelist += container.children
>                  continue
>
> -            snode = self._make_section(section.tag)
> +            snode = self._make_section(section.kind.name.title())
>              self._parse_text_into_node(dedent(section.text), snode)
>              nodelist.append(snode)
>          return nodelist
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index 36cb64a677a..c3004aa70c6 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -15,6 +15,7 @@
>  # See the COPYING file in the top-level directory.
>
>  from collections import OrderedDict
> +import enum
>  import os
>  import re
>  from typing import (
> @@ -575,7 +576,10 @@ def get_doc(self) -> 'QAPIDoc':
>                          )
>                          raise QAPIParseError(self, emsg)
>
> -                    doc.new_tagged_section(self.info, match.group(1))
> +                    doc.new_tagged_section(
> +                        self.info,
> +                        QAPIDoc.Kind.from_string(match.group(1))
> +                    )
>                      text = line[match.end():]
>                      if text:
>                          doc.append_line(text)
> @@ -586,7 +590,7 @@ def get_doc(self) -> 'QAPIDoc':
>                          self,
>                          "unexpected '=' markup in definition documentation")
>                  else:
> -                    # tag-less paragraph
> +                    # plain paragraph(s)

We're parsing a single pargraph here.  The plain section we add it to
may have any number of paragraphs.  But for me, the comment is about
what's being parsed.  Mind to drop (s)?

>                      doc.ensure_untagged_section(self.info)
>                      doc.append_line(line)
>                      line = self.get_doc_paragraph(doc)
> @@ -635,14 +639,37 @@ class QAPIDoc:
>      Free-form documentation blocks consist only of a body section.
>      """
>
> +    class Kind(enum.Enum):
> +        PLAIN = 0
> +        MEMBER = 1
> +        FEATURE = 2
> +        RETURNS = 3
> +        ERRORS = 4
> +        SINCE = 5
> +        TODO = 6
> +
> +        @staticmethod
> +        def from_string(kind: str) -> 'QAPIDoc.Kind':

Remind me, why do we need to quote the type here?

> +            return QAPIDoc.Kind[kind.upper()]
> +
> +        def text_required(self) -> bool:
> +            # Only "plain" sections can be empty
> +            return self.value not in (0,)

Rather roundabout way to check for PLAIN, isn't it?

There's just one caller (see below).  I doubt the method is worth its
keep.

> +
> +        def __str__(self) -> str:
> +            return self.name.title()

I wonder whether a simple StrEnum without methods would do.  Oh, StrEnum
is new in 3.11.  Nevermind.

Hmm.

    >>> Kind = Enum('Kind', [('PLAIN', 'Plain'), ('TODO, 'TODO)])
    >>> kind=Kind('Plain')
    >>> kind.value
    'Plain'

What do you think?

> +
>      class Section:
>          # pylint: disable=too-few-public-methods
> -        def __init__(self, info: QAPISourceInfo,
> -                     tag: Optional[str] = None):
> +        def __init__(
> +            self,
> +            info: QAPISourceInfo,
> +            kind: 'QAPIDoc.Kind',
> +        ):
>              # section source info, i.e. where it begins
>              self.info = info
> -            # section tag, if any ('Returns', '@name', ...)
> -            self.tag = tag
> +            # section kind
> +            self.kind = kind
>              # section text without tag
>              self.text = ''
>
> @@ -650,8 +677,14 @@ def append_line(self, line: str) -> None:
>              self.text += line + '\n'
>
>      class ArgSection(Section):
> -        def __init__(self, info: QAPISourceInfo, tag: str):
> -            super().__init__(info, tag)
> +        def __init__(
> +            self,
> +            info: QAPISourceInfo,
> +            kind: 'QAPIDoc.Kind',
> +            name: str
> +        ):
> +            super().__init__(info, kind)
> +            self.name = name
>              self.member: Optional['QAPISchemaMember'] = None

Before the patch, use of a separate type for members, arguments and
features was necessary to distinguish between '@TAG:' and 'TAG:' for the
various TAGs.  This is no longer the case.  Fold ArgSection into
Section?  Not sure.  If yes, separate patch to keep this one as
mechanical as possible.

>
>          def connect(self, member: 'QAPISchemaMember') -> None:
> @@ -663,7 +696,9 @@ def __init__(self, info: QAPISourceInfo, symbol: Optional[str] = None):
>          # definition doc's symbol, None for free-form doc
>          self.symbol: Optional[str] = symbol
>          # the sections in textual order
> -        self.all_sections: List[QAPIDoc.Section] = [QAPIDoc.Section(info)]
> +        self.all_sections: List[QAPIDoc.Section] = [
> +            QAPIDoc.Section(info, QAPIDoc.Kind.PLAIN)
> +        ]
>          # the body section
>          self.body: Optional[QAPIDoc.Section] = self.all_sections[0]
>          # dicts mapping parameter/feature names to their description
> @@ -680,12 +715,17 @@ def __init__(self, info: QAPISourceInfo, symbol: Optional[str] = None):
>      def end(self) -> None:
>          for section in self.all_sections:
>              section.text = section.text.strip('\n')
> -            if section.tag is not None and section.text == '':
> +            if section.kind.text_required() and section.text == '':

This is the only use of .text_required().  I believe checking for PLAIN
would be clearer.

>                  raise QAPISemError(
> -                    section.info, "text required after '%s:'" % section.tag)
> +                    section.info, "text required after '%s:'" % section.kind)
>  
> -    def ensure_untagged_section(self, info: QAPISourceInfo) -> None:
> -        if self.all_sections and not self.all_sections[-1].tag:
> +    def ensure_untagged_section(
> +        self,
> +        info: QAPISourceInfo,
> +    ) -> None:

Accidental line breaking?

> +        kind = QAPIDoc.Kind.PLAIN
> +
> +        if self.all_sections and self.all_sections[-1].kind == kind:

I'd prefer not to hide PLAIN behind a variable, but I'd also prefer
the condition to fit on a line.  Hmm.

>              # extend current section
>              section = self.all_sections[-1]
>              if not section.text:

Maybe

           section = self.all_sections[-1] if self.all_sections else None

           if second and section.kind = QAPIDoc.Kind.Plain:
               # extend current section
               if not section.text:

> @@ -693,46 +733,56 @@ def ensure_untagged_section(self, info: QAPISourceInfo) -> None:
>                  section.info = info
>              section.text += '\n'
>              return
> +
>          # start new section
> -        section = self.Section(info)
> +        section = self.Section(info, kind)
>          self.sections.append(section)
>          self.all_sections.append(section)
>
> -    def new_tagged_section(self, info: QAPISourceInfo, tag: str) -> None:
> -        section = self.Section(info, tag)
> -        if tag == 'Returns':
> +    def new_tagged_section(
> +        self,
> +        info: QAPISourceInfo,
> +        kind: 'QAPIDoc.Kind',
> +    ) -> None:
> +        section = self.Section(info, kind)
> +        if kind == QAPIDoc.Kind.RETURNS:
>              if self.returns:
>                  raise QAPISemError(
> -                    info, "duplicated '%s' section" % tag)
> +                    info, "duplicated '%s' section" % kind)
>              self.returns = section
> -        elif tag == 'Errors':
> +        elif kind == QAPIDoc.Kind.ERRORS:
>              if self.errors:
>                  raise QAPISemError(
> -                    info, "duplicated '%s' section" % tag)
> +                    info, "duplicated '%s' section" % kind)
>              self.errors = section
> -        elif tag == 'Since':
> +        elif kind == QAPIDoc.Kind.SINCE:
>              if self.since:
>                  raise QAPISemError(
> -                    info, "duplicated '%s' section" % tag)
> +                    info, "duplicated '%s' section" % kind)
>              self.since = section
>          self.sections.append(section)
>          self.all_sections.append(section)
>
> -    def _new_description(self, info: QAPISourceInfo, name: str,
> -                         desc: Dict[str, ArgSection]) -> None:
> +    def _new_description(
> +        self,
> +        info: QAPISourceInfo,
> +        name: str,
> +        kind: 'QAPIDoc.Kind',
> +        desc: Dict[str, ArgSection]
> +    ) -> None:
>          if not name:
>              raise QAPISemError(info, "invalid parameter name")
>          if name in desc:
>              raise QAPISemError(info, "'%s' parameter name duplicated" % name)
> -        section = self.ArgSection(info, '@' + name)
> +        section = self.ArgSection(info, kind, name)
>          self.all_sections.append(section)
>          desc[name] = section
>
>      def new_argument(self, info: QAPISourceInfo, name: str) -> None:
> -        self._new_description(info, name, self.args)
> +        self._new_description(info, name, QAPIDoc.Kind.MEMBER, self.args)
>
>      def new_feature(self, info: QAPISourceInfo, name: str) -> None:
> -        self._new_description(info, name, self.features)
> +        self._new_description(info, name, QAPIDoc.Kind.FEATURE, self.features)

QAPIDoc.Kind.FOO is a mouthful, and it tends to result in long lines,
like here.  Can't see an easy and clean way to reduce the verbosity.

>
>      def append_line(self, line: str) -> None:
>          self.all_sections[-1].append_line(line)
> @@ -744,8 +794,9 @@ def connect_member(self, member: 'QAPISchemaMember') -> None:
>                  raise QAPISemError(member.info,
>                                     "%s '%s' lacks documentation"
>                                     % (member.role, member.name))
> -            self.args[member.name] = QAPIDoc.ArgSection(
> -                self.info, '@' + member.name)
> +            section = QAPIDoc.ArgSection(
> +                self.info, QAPIDoc.Kind.MEMBER, member.name)
> +            self.args[member.name] = section

Why the extra variable?

>          self.args[member.name].connect(member)
>
>      def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
> diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
> index ec277be91e9..2d33a305ee7 100644
> --- a/tests/qapi-schema/doc-good.out
> +++ b/tests/qapi-schema/doc-good.out
> @@ -110,7 +110,7 @@ The _one_ {and only}, description on the same line
>  Also _one_ {and only}
>      feature=enum-member-feat
>  a member feature
> -    section=None
> +    section=Plain
>  @two is undocumented
>  doc symbol=Base
>      body=
> @@ -168,15 +168,15 @@ description starts on the same line
>  a feature
>      feature=cmd-feat2
>  another feature
> -    section=None
> +    section=Plain
>  .. note:: @arg3 is undocumented
>      section=Returns
>  @Object
>      section=Errors
>  some
> -    section=TODO
> +    section=Todo

With the method-less Enum I suggested, this hunk would go away.  Not
that it matters :)

>  frobnicate
> -    section=None
> +    section=Plain
>  .. admonition:: Notes
>
>   - Lorem ipsum dolor sit amet
> @@ -209,7 +209,7 @@ If you're bored enough to read this, go see a video of boxed cats
>  a feature
>      feature=cmd-feat2
>  another feature
> -    section=None
> +    section=Plain
>  .. qmp-example::
>
>     -> "this example"
> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
> index 7e3f9f4aa1f..bca924309be 100755
> --- a/tests/qapi-schema/test-qapi.py
> +++ b/tests/qapi-schema/test-qapi.py
> @@ -131,7 +131,7 @@ def test_frontend(fname):
>          for feat, section in doc.features.items():
>              print('    feature=%s\n%s' % (feat, section.text))
>          for section in doc.sections:
> -            print('    section=%s\n%s' % (section.tag, section.text))
> +            print('    section=%s\n%s' % (section.kind, section.text))
>
>
>  def open_test_result(dir_name, file_name, update):

[PATCH 08/10] qapi/schema: add __repr__ to QAPIDoc.Section

[John Snow]

Makes debugging far more pleasant when you can just print(section) and
get something reasonable to display.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/parser.py | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index c3004aa70c6..617711b1f04 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -673,6 +673,9 @@ def __init__(
             # section text without tag
             self.text = ''
 
+        def __repr__(self) -> str:
+            return f"<QAPIDoc.Section kind={self.kind!r} text={self.text!r}>"
+
         def append_line(self, line: str) -> None:
             self.text += line + '\n'
 
@@ -687,6 +690,12 @@ def __init__(
             self.name = name
             self.member: Optional['QAPISchemaMember'] = None
 
+        def __repr__(self) -> str:
+            return (
+                f"<QAPIDoc.ArgSection kind={self.kind!r} "
+                f"name={self.name!r} text={self.text!r}>"
+            )
+
         def connect(self, member: 'QAPISchemaMember') -> None:
             self.member = member
 
-- 
2.48.1

[PATCH 09/10] qapi/source: allow multi-line QAPISourceInfo advancing

[John Snow]

This is for the sake of the new rST generator (the "transmogrifier") so
we can advance multiple lines on occasion while keeping the
generated<-->source mappings accurate.

next_line now simply takes an optional n parameter which chooses the
number of lines to advance.

This is used mainly when converting section syntax in free-form
documentation to more traditional rST section header syntax, which
does not always line up 1:1 for line counts.

For example:

```
 ##
 # = Section     <-- Info is pointing here, "L1"
 #
 # Lorem Ipsum
 ##
```

would be transformed to rST by the transmogrifier as:

```
=======        <-- L1
Section        <-- L1
=======        <-- L1
               <-- L2
Lorem Ipsum    <-- L3
```

With each line having its own source information credited to different
source lines; sometimes one line to many.

After consuming the single "Section" line from the source, we want to
advance the source pointer to the next non-empty line which requires
jumping by more than one line.

```
 ##
 # = Section     <-- Info is pointing here, "L1"
 #
 # Lorem Ipsum   <-- We want to jump to here, +2.
 ##
```

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/source.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
index 7b379fdc925..ffdc3f482ac 100644
--- a/scripts/qapi/source.py
+++ b/scripts/qapi/source.py
@@ -47,9 +47,9 @@ def set_defn(self, meta: str, name: str) -> None:
         self.defn_meta = meta
         self.defn_name = name
 
-    def next_line(self: T) -> T:
+    def next_line(self: T, n: int = 1) -> T:
         info = copy.copy(self)
-        info.line += 1
+        info.line += n
         return info
 
     def loc(self) -> str:
-- 
2.48.1

[PATCH 10/10] docs: disambiguate cross-references

[John Snow]

These references are not in and of themselves problems at the moment,
however, the forthcoming qapidoc conversion series will begin adding
cross-reference targets that conflict with these existing targets.

As a result, cross-references such as "migration", "qom", "replay" will
become ambiguous. Modify these references to be more explicit to prevent
"ambiguous cross-reference" warnings in the future.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 docs/devel/codebase.rst |  6 +++---
 docs/glossary.rst       | 10 +++++-----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/docs/devel/codebase.rst b/docs/devel/codebase.rst
index 4039875ee04..1b09953197b 100644
--- a/docs/devel/codebase.rst
+++ b/docs/devel/codebase.rst
@@ -23,7 +23,7 @@ Some of the main QEMU subsystems are:
 - `Devices<device-emulation>` & Board models
 - `Documentation <documentation-root>`
 - `GDB support<GDB usage>`
-- `Migration<migration>`
+- :ref:`Migration<migration>`
 - `Monitor<QEMU monitor>`
 - :ref:`QOM (QEMU Object Model)<qom>`
 - `System mode<System emulation>`
@@ -112,7 +112,7 @@ yet, so sometimes the source code is all you have.
 * `libdecnumber <https://gitlab.com/qemu-project/qemu/-/tree/master/libdecnumber>`_:
   Import of gcc library, used to implement decimal number arithmetic.
 * `migration <https://gitlab.com/qemu-project/qemu/-/tree/master/migration>`__:
-  `Migration framework <migration>`.
+  :ref:`Migration framework <migration>`.
 * `monitor <https://gitlab.com/qemu-project/qemu/-/tree/master/monitor>`_:
   `Monitor <QEMU monitor>` implementation (HMP & QMP).
 * `nbd <https://gitlab.com/qemu-project/qemu/-/tree/master/nbd>`_:
@@ -193,7 +193,7 @@ yet, so sometimes the source code is all you have.
   - `lcitool <https://gitlab.com/qemu-project/qemu/-/tree/master/tests/lcitool>`_:
     Generate dockerfiles for CI containers.
   - `migration <https://gitlab.com/qemu-project/qemu/-/tree/master/tests/migration>`_:
-    Test scripts and data for `Migration framework <migration>`.
+    Test scripts and data for :ref:`Migration framework <migration>`.
   - `multiboot <https://gitlab.com/qemu-project/qemu/-/tree/master/tests/multiboot>`_:
     Test multiboot functionality for x86_64/i386.
   - `qapi-schema <https://gitlab.com/qemu-project/qemu/-/tree/master/tests/qapi-schema>`_:
diff --git a/docs/glossary.rst b/docs/glossary.rst
index 693d9855dd1..4fa044bfb6e 100644
--- a/docs/glossary.rst
+++ b/docs/glossary.rst
@@ -120,7 +120,7 @@ Migration
 ---------
 
 QEMU can save and restore the execution of a virtual machine between different
-host systems. This is provided by the `Migration framework<migration>`.
+host systems. This is provided by the :ref:`Migration framework<migration>`.
 
 NBD
 ---
@@ -212,14 +212,14 @@ machine emulator and virtualizer.
 QOM
 ---
 
-`QEMU Object Model <qom>` is an object oriented API used to define various
-devices and hardware in the QEMU codebase.
+:ref:`QEMU Object Model <qom>` is an object oriented API used to define
+various devices and hardware in the QEMU codebase.
 
 Record/replay
 -------------
 
-`Record/replay <replay>` is a feature of QEMU allowing to have a deterministic
-and reproducible execution of a virtual machine.
+:ref:`Record/replay <replay>` is a feature of QEMU allowing to have a
+deterministic and reproducible execution of a virtual machine.
 
 Rust
 ----
-- 
2.48.1

Re: [PATCH 00/10] qapi: misc testing and doc patches

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> GitLab: https://gitlab.com/jsnow/qemu/-/pipelines/1684045409
>
> This is a series of random qapi and documentation patches;
>
> Patches 1-3: Formalize QAPI python testing (finally ...)
> Patches 4-10: Miscellaneous patches and fixes related to the
> documentation overhaul project; but were standalone enough that they
> could be excised from a fairly lengthy series ...
>
> Markus: I think everything here is suitable for merging, even though
> some of it might lack a little context. The more I pull out of that big
> series, the easier that series will be to review :)
>
> Please stage as much as you feel comfortable doing, you can skip any
> individual patch at your discretion.

I'm queuing the no-brainers immediately: PATCH 01,02,04,06

I had a question on PATCH 03, and I'm holding the patch for now.

I'm not sure I like PATCH 05 without a case where it shows improvement.
I tried to find one, but no luck, yet.  Holding for now.

I still need to review PATCH 07 in detail.  Holding for now.

PATCH 08 depends on it.  Holding for now.

PATCH 09 has no use, yet.  I'd prefer you resubmit it together with
something that uses it, at your convenience.

Similar for PATCH 10.

I expect to queue the patches I'm holding, i.e. 03,05,07,08, in due
time.

Thanks!