[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 19/20] qapi: convert "Note" sections to plain rST
|
From: |
Markus Armbruster |
|
Subject: |
Re: [PATCH 19/20] qapi: convert "Note" sections to plain rST |
|
Date: |
Tue, 18 Jun 2024 13:08:09 +0200 |
|
User-agent: |
Gnus/5.13 (Gnus v5.13) |
John Snow <jsnow@redhat.com> writes:
> On Fri, Jun 14, 2024, 9:44 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@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 <jsnow@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
>> > ##
[...]