John Snow <js...@redhat.com> writes: > On Fri, Jun 14, 2024, 9:44 AM Markus Armbruster <arm...@redhat.com> wrote: > >> John Snow <js...@redhat.com> writes: >> >> > We do not need a dedicated section for notes. By eliminating a specially >> > parsed section, these notes can be treated as normal rST paragraphs in >> > the new QMP reference manual, and can be placed and styled much more >> > flexibly. >> > >> > Update the QAPI parser to now prohibit special "Note" sections while >> > suggesting a new syntax. >> > >> > The exact formatting to use is a matter of taste, but a good candidate >> > is simply: >> > >> > .. note:: lorem ipsum ... >> > >> > but there are other choices, too. The Sphinx readthedocs theme offers >> > theming for the following forms (capitalization unimportant); all are >> > adorned with a (!) symbol in the title bar for rendered HTML docs. >> > > Store this paragraph in your L1 cache for a moment ... > >> >> > These are rendered in orange: >> > >> > .. Attention:: ... >> > .. Caution:: ... >> > .. WARNING:: ... >> > >> > These are rendered in red: >> > >> > .. DANGER:: ... >> > .. Error:: ... >> > >> > These are rendered in green: >> > >> > .. Hint:: ... >> > .. Important:: ... >> > .. Tip:: ... >> > >> > These are rendered in blue: >> > >> > .. Note:: ... >> > .. admonition:: custom title >> > >> > admonition body text >> > >> > This patch uses ".. notes::" almost everywhere, with just two "caution" >> > directives. ".. admonition:: notes" is used in a few places where we had >> > an ordered list of multiple notes. >> > >> > Signed-off-by: John Snow <js...@redhat.com> >> > --- >> > qapi/block-core.json | 30 +++---- >> > qapi/block.json | 2 +- >> > qapi/char.json | 12 +-- >> > qapi/control.json | 15 ++-- >> > qapi/dump.json | 2 +- >> > qapi/introspect.json | 6 +- >> > qapi/machine-target.json | 26 +++--- >> > qapi/machine.json | 47 +++++----- >> > qapi/migration.json | 12 +-- >> > qapi/misc.json | 88 +++++++++---------- >> > qapi/net.json | 6 +- >> > qapi/pci.json | 7 +- >> > qapi/qdev.json | 30 +++---- >> > qapi/qom.json | 19 ++-- >> > qapi/rocker.json | 16 ++-- >> > qapi/run-state.json | 18 ++-- >> > qapi/sockets.json | 10 +-- >> > qapi/stats.json | 22 ++--- >> > qapi/transaction.json | 8 +- >> > qapi/ui.json | 29 +++--- >> > qapi/virtio.json | 12 +-- >> > qga/qapi-schema.json | 48 +++++----- >> > scripts/qapi/parser.py | 9 ++ >> > tests/qapi-schema/doc-empty-section.err | 2 +- >> > tests/qapi-schema/doc-empty-section.json | 2 +- >> > tests/qapi-schema/doc-good.json | 6 +- >> > tests/qapi-schema/doc-good.out | 10 ++- >> > tests/qapi-schema/doc-good.txt | 14 ++- >> > .../qapi-schema/doc-interleaved-section.json | 2 +- >> > 29 files changed, 258 insertions(+), 252 deletions(-) >> >> Missing: update of docs/devel/qapi-code-gen.rst. Something like >> >> diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst >> index f453bd3546..1a4e240adb 100644 >> --- a/docs/devel/qapi-code-gen.rst >> +++ b/docs/devel/qapi-code-gen.rst >> @@ -995,14 +995,13 @@ line "Features:", like this:: >> # @feature: Description text >> >> A tagged section begins with a paragraph that starts with one of the >> -following words: "Note:"/"Notes:", "Since:", "Example:"/"Examples:", >> -"Returns:", "Errors:", "TODO:". It ends with the start of a new >> -section. >> +following words: "Since:", "Example:"/"Examples:", "Returns:", >> +"Errors:", "TODO:". It ends with the start of a new section. >> >> The second and subsequent lines of tagged sections must be indented >> like this:: >> >> - # Note: Ut enim ad minim veniam, quis nostrud exercitation ullamco >> + # TODO: Ut enim ad minim veniam, quis nostrud exercitation ullamco >> # laboris nisi ut aliquip ex ea commodo consequat. >> # >> # Duis aute irure dolor in reprehenderit in voluptate velit esse >> >> > Done.
Thanks! >> > >> > diff --git a/qapi/block-core.json b/qapi/block-core.json >> > index 64fe5240cc9..530af40404d 100644 >> > --- a/qapi/block-core.json >> > +++ b/qapi/block-core.json >> > @@ -1510,7 +1510,7 @@ >> > # @mode: whether and how QEMU should create a new image, default is >> > # 'absolute-paths'. >> > # >> > -# Note: Either @device or @node-name must be set but not both. >> > +# .. note:: Either @device or @node-name must be set but not both. >> >> The commit message talks about ".. Note::", but you actually use >> ".. note::". Is there a difference? >> > > Retrieve paragraph from L1 cache. > > Nope! Sphinx RTD theme docs use capitals sporadically, but it's case > insensitive. I stuck with all lowercase everywhere in the patches, but the > capitalization in the commit message came directly from the Sphinx RTD > theme documentation. Lowercase seems to be consistent with existing usage elsewhere. I'd stick to lowercase in the commit message as well. Matter of taste, you decide. >> > # >> > ## >> > { 'struct': 'BlockdevSnapshotSync', >> > @@ -1616,9 +1616,9 @@ >> > # >> > # @unstable: Member @x-perf is experimental. >> > # >> > -# Note: @on-source-error and @on-target-error only affect background >> > -# I/O. If an error occurs during a guest write request, the >> > -# device's rerror/werror actions will be used. >> > +# .. note:: @on-source-error and @on-target-error only affect background >> > +# I/O. If an error occurs during a guest write request, the device's >> > +# rerror/werror actions will be used. >> > # >> > # Since: 4.2 >> > ## >> > @@ -5534,8 +5534,8 @@ >> > # after this event and must be repaired (Since 2.2; before, every >> > # BLOCK_IMAGE_CORRUPTED event was fatal) >> > # >> > -# Note: If action is "stop", a STOP event will eventually follow the >> > -# BLOCK_IO_ERROR event. >> > +# .. note:: If action is "stop", a STOP event will eventually follow the >> > +# BLOCK_IO_ERROR event. >> > # >> > # Example: >> > # >> > @@ -5581,8 +5581,8 @@ >> > # field is a debugging aid for humans, it should not be parsed by >> > # applications) (since: 2.2) >> > # >> > -# Note: If action is "stop", a STOP event will eventually follow the >> > -# BLOCK_IO_ERROR event >> > +# .. note:: If action is "stop", a STOP event will eventually follow the >> > +# BLOCK_IO_ERROR event. >> >> You're adding a period. Okay, but please mention it in the commit >> message. >> > > OK. When hoisting notes into little visual boxes I felt it looked naked > without the punctuation, so I just went for it. Sorry. You're right, and I appreciate your attention to detail. >> > # >> > # Since: 0.13 >> > # >> > @@ -5720,8 +5720,8 @@ >> > # >> > # @speed: rate limit, bytes per second >> > # >> > -# Note: The "ready to complete" status is always reset by a >> > -# @BLOCK_JOB_ERROR event >> > +# .. note:: The "ready to complete" status is always reset by a >> > +# @BLOCK_JOB_ERROR event. >> >> Likewise. Not going to point this out again. >> > > I just need to update the commit message, yeah?. Either that, or you add the periods in a separate patch, possibly together with related changes. >> > # >> > # Since: 1.3 >> > # >> > @@ -5974,7 +5974,7 @@ >> > # >> > # @sectors-count: failed read operation sector count >> > # >> > -# Note: This event is rate-limited. >> > +# .. note:: This event is rate-limited. >> > # >> > # Since: 2.0 >> > # >> > @@ -6005,7 +6005,7 @@ >> > # >> > # @sectors-count: failed read operation sector count >> > # >> > -# Note: This event is rate-limited. >> > +# .. note:: This event is rate-limited. >> > # >> > # Since: 2.0 >> > # >> > @@ -6037,9 +6037,9 @@ >> > # >> > # @name: the name of the internal snapshot to be created >> > # >> > -# Notes: In transaction, if @name is empty, or any snapshot matching >> > -# @name exists, the operation will fail. Only some image formats >> > -# support it, for example, qcow2, and rbd. >> > +# .. note:: In a transaction, if @name is empty or any snapshot matching >> > +# @name exists, the operation will fail. Only some image formats >> > +# support it; for example, qcow2, and rbd. >> >> You're fixing up prose. Welcome, but put it in a separate patch, >> please, to keep this one mechanical. >> > > Couldn't help it while auditing every last notebox. (: Again, I appreciate your attention to detail. > OK, separate patch... > > >> > # >> > # Since: 1.7 >> > ## >> > diff --git a/qapi/block.json b/qapi/block.json >> > index 5de99fe09d9..ea81d9e1921 100644 >> > --- a/qapi/block.json >> > +++ b/qapi/block.json >> > @@ -113,7 +113,7 @@ >> > # Errors: >> > # - If @device is not a valid block device, DeviceNotFound >> > # >> > -# Notes: Ejecting a device with no media results in success >> > +# .. note:: Ejecting a device with no media results in success. >> > # >> > # Since: 0.14 >> > # >> > diff --git a/qapi/char.json b/qapi/char.json >> > index ab4c23976ed..0f39c2d5cdf 100644 >> > --- a/qapi/char.json >> > +++ b/qapi/char.json >> > @@ -21,8 +21,8 @@ >> > # backend (e.g. with the chardev=... option) is in open or closed >> > # state (since 2.1) >> > # >> > -# Notes: @filename is encoded using the QEMU command line character >> > -# device encoding. See the QEMU man page for details. >> > +# .. note:: @filename is encoded using the QEMU command line character >> > +# device encoding. See the QEMU man page for details. >> > # >> > # Since: 0.14 >> > ## >> > @@ -387,9 +387,9 @@ >> > # >> > # @rows: console height, in chars >> > # >> > -# Note: the options are only effective when the VNC or SDL graphical >> > -# display backend is active. They are ignored with the GTK, >> > -# Spice, VNC and D-Bus display backends. >> > +# .. note:: The options are only effective when the VNC or SDL graphical >> > +# display backend is active. They are ignored with the GTK, Spice, >> > +# VNC and D-Bus display backends. >> >> You're capitalizing the beginning of the note text. Good, because the >> generated HTML wants that. But please mention it in the commit message. >> >> More below; not going to point it out. >> > > OK; so long as the commit message mentions it, we don't need to make note > of each time I do it, right...? A blanket notice should suffice. Something like "Tidy up note text to start with a capital letter and end with a period, because the formatted documentation clearly looks better that way." >> > # >> > # Since: 1.5 >> > ## >> > @@ -805,7 +805,7 @@ >> > # >> > # @open: true if the guest has opened the virtio-serial port >> > # >> > -# Note: This event is rate-limited. >> > +# .. note:: This event is rate-limited. >> > # >> > # Since: 2.1 >> > # >> > diff --git a/qapi/control.json b/qapi/control.json >> > index 10c906fa0e7..2498e5dd6ba 100644 >> > --- a/qapi/control.json >> > +++ b/qapi/control.json >> > @@ -22,14 +22,13 @@ >> > # "arguments": { "enable": [ "oob" ] } } >> > # <- { "return": {} } >> > # >> > -# Notes: This command is valid exactly when first connecting: it must >> > -# be issued before any other command will be accepted, and will >> > -# fail once the monitor is accepting other commands. (see qemu >> > -# docs/interop/qmp-spec.rst) >> > +# .. note:: This command is valid exactly when first connecting: it must >> > +# be issued before any other command will be accepted, and will fail >> > +# once the monitor is accepting other commands. >> > +# (see :doc:`/interop/qmp-spec`) >> >> You're adding markup to make a reference work. Welcome, but put it in a >> separate patch, please, to keep this one mechanical. >> > > Whoops. This snuck in. I do have a growing markup change patch... > > >> > # >> > -# The QMP client needs to explicitly enable QMP capabilities, >> > -# otherwise all the QMP capabilities will be turned off by >> > -# default. >> > +# .. note:: The QMP client needs to explicitly enable QMP capabilities, >> > +# otherwise all the QMP capabilities will be turned off by default. >> > # >> > # Since: 0.13 >> > ## >> > @@ -150,7 +149,7 @@ >> > # ] >> > # } >> > # >> > -# Note: This example has been shortened as the real response is too >> > +# Note: This example has been shortened as the real response is too >> > # long. >> >> Your commit message lists a number of conversions. This one isn't among >> them. >> >> The next patch reverts the indentation and drops "Note: ": >> >> -# Note: This example has been shortened as the real response is too >> -# long. >> +# This example has been shortened as the real response is too long. >> >> Hmm. Swap the two patches? Perhaps not worth the bother now. Squash >> the next patch's change into this one? >> > > Yeah, OK. (The problem was that this began being picked up as a note > section in its own right, but I thought it wasn't appropriate in this case, > but needed to avoid the parser complaining about the old Note: section.) > > >> A few more below. >> >> > ## >> > { 'command': 'query-commands', 'returns': ['CommandInfo'], [...] >> > diff --git a/qapi/misc.json b/qapi/misc.json >> > index 4b41e15dcd4..b04efbadec6 100644 >> > --- a/qapi/misc.json >> > +++ b/qapi/misc.json >> > @@ -103,9 +103,9 @@ >> > # >> > # Returns a list of information about each iothread. >> > # >> > -# Note: this list excludes the QEMU main loop thread, which is not >> > -# declared using the -object iothread command-line option. It is >> > -# always the main thread of the process. >> > +# .. note:: This list excludes the QEMU main loop thread, which is not >> > +# declared using the ``-object iothread`` command-line option. It is >> > +# always the main thread of the process. >> >> You're adding markup. Welcome, but put it in a separate patch, please, >> to keep this one mechanical. >> > > OK [...] >> > @@ -1461,16 +1462,15 @@ >> > # * POSIX: as defined by os-release(5) >> > # * Windows: contains string "server" or "client" >> > # >> > -# Notes: On POSIX systems the fields @id, @name, @pretty-name, >> > -# @version, @version-id, @variant and @variant-id follow the >> > -# definition specified in os-release(5). Refer to the manual page >> > -# for exact description of the fields. Their values are taken >> > -# from the os-release file. If the file is not present in the >> > -# system, or the values are not present in the file, the fields >> > -# are not included. >> > +# .. note:: On POSIX systems the fields @id, @name, @pretty-name, >> > +# @version, @version-id, @variant and @variant-id follow the >> > +# definition specified in os-release(5). Refer to the manual page for >> > +# exact description of the fields. Their values are taken from the >> > +# os-release file. If the file is not present in the system, or the >> > +# values are not present in the file, the fields are not included. >> > # >> > -# On Windows the values are filled from information gathered from >> > -# the system. >> > +# On Windows the values are filled from information gathered from >> > +# the system. >> >> Accidental indentation change? > > I don't think so; the original heading seems to suggest that there is a > sequence of notes; "On POSIX" ... "On Windows". This indentation change > keeps this information in the same note box. Still in the same box if I instead keep the indentation like this: @@ -1461,7 +1462,7 @@ # * POSIX: as defined by os-release(5) # * Windows: contains string "server" or "client" # -# Notes: On POSIX systems the fields @id, @name, @pretty-name, +# .. note:: On POSIX systems the fields @id, @name, @pretty-name, # @version, @version-id, @variant and @variant-id follow the # definition specified in os-release(5). Refer to the manual page # for exact description of the fields. Their values are taken >> > # >> > # Since: 2.10 >> > ## [...]