Re: [PATCH 09/13] qapi: convert "Note" sections to plain rST

[Markus Armbruster]

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.
>
> Convert all existing "Note" and "Notes" sections to pure rST. As part of
> the conversion, capitalize the first letter of each sentence and add
> trailing punctuation where appropriate to ensure notes look sensible and
> consistent in rendered HTML documentation.
>
> Update docs/devel/qapi-code-gen.rst to reflect the new paradigm, and ...
>
> ... Update the QAPI parser to prohibit "Note" sections while suggesting

Scratch "... ..." and downcase "Update"?

> 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.
>
> See
> https://sphinx-rtd-theme.readthedocs.io/en/stable/demo/demo.html#admonitions
> for examples of each directive/admonition in use.
>
> 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 ".. note::" almost everywhere, with just two "caution"
> directives. ".. admonition:: notes" is used in a few places where we had
> an ordered list of multiple notes that would not make sense as
> standalone/separate admonitions.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Acked-by: Stefan Hajnoczi <stefanha@redhat.com> [for block*.json]

[...]

> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index b3de1fb6b3a..57598331c5c 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -422,8 +422,9 @@
>  # Returns: GuestFsfreezeStatus ("thawed", "frozen", etc., as defined
>  #     below)
>  #
> -# Note: This may fail to properly report the current state as a result
> -#     of some other guest processes having issued an fs freeze/thaw.
> +# .. note:: This may fail to properly report the current state as a
> +#    result of some other guest processes having issued an fs
> +#    freeze/thaw.
>  #
>  # Since: 0.15.0
>  ##
> @@ -443,9 +444,9 @@
>  #
>  # Returns: Number of file systems currently frozen.
>  #
> -# Note: On Windows, the command is implemented with the help of a
> -#     Volume Shadow-copy Service DLL helper.  The frozen state is
> -#     limited for up to 10 seconds by VSS.
> +# .. note:: On Windows, the command is implemented with the help of a
> +#    Volume Shadow-copy Service DLL helper.  The frozen state is limited
> +#    for up to 10 seconds by VSS.
>  #
>  # Since: 0.15.0
>  ##
> @@ -479,10 +480,10 @@
>  #
>  # Returns: Number of file systems thawed by this call
>  #
> -# Note: if return value does not match the previous call to
> -#     guest-fsfreeze-freeze, this likely means some freezable
> -#     filesystems were unfrozen before this call, and that the
> -#     filesystem state may have changed before issuing this command.
> +# .. note:: If return value does not match the previous call to
> +#    guest-fsfreeze-freeze, this likely means some freezable filesystems
> +#    were unfrozen before this call, and that the filesystem state may
> +#    have changed before issuing this command.
>  #
>  # Since: 0.15.0
>  ##
> @@ -560,8 +561,8 @@
>  # Errors:
>  #     - If suspend to disk is not supported, Unsupported
>  #
> -# Notes: It's strongly recommended to issue the guest-sync command
> -#     before sending commands when the guest resumes
> +# .. note:: It's strongly recommended to issue the guest-sync command
> +#    before sending commands when the guest resumes.
>  #
>  # Since: 1.1
>  ##
> @@ -596,8 +597,8 @@
>  # Errors:
>  #     - If suspend to ram is not supported, Unsupported
>  #
> -# Notes: It's strongly recommended to issue the guest-sync command
> -#     before sending commands when the guest resumes
> +# .. note:: It's strongly recommended to issue the guest-sync command
> +#    before sending commands when the guest resumes.
>  #
>  # Since: 1.1
>  ##
> @@ -631,8 +632,8 @@
>  # Errors:
>  #     - If hybrid suspend is not supported, Unsupported
>  #
> -# Notes: It's strongly recommended to issue the guest-sync command
> -#     before sending commands when the guest resumes
> +# .. note:: It's strongly recommended to issue the guest-sync command
> +#    before sending commands when the guest resumes.
>  #
>  # Since: 1.1
>  ##
> @@ -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.

Please don't change the indentation here.  I get the same output with

  @@ -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
>  ##
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index dfd6a6c5bee..53b06a94508 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -548,6 +548,21 @@ def get_doc(self) -> 'QAPIDoc':
>                          r'(Returns|Errors|Since|Notes?|Examples?|TODO): *',
>                          line):
>                      # tagged section
> +
> +                    # TODO: Remove this error sometime in 2025 or so
> +                    # after we've fully transitioned to the new qapidoc
> +                    # generator.
> +
> +                    # See commit message for more markup suggestions O:-)
> +                    if 'Note' in match.group(1):
> +                        emsg = (
> +                            f"The '{match.group(1)}' section is no longer "
> +                            "supported. Please use rST's '.. note::' or "
> +                            "'.. admonition:: notes' directives, or another "
> +                            "suitable admonition instead."
> +                        )
> +                        raise QAPIParseError(self, emsg)
> +
>                      doc.new_tagged_section(self.info, match.group(1))
>                      text = line[match.end():]
>                      if text:
> diff --git a/tests/qapi-schema/doc-empty-section.err 
> b/tests/qapi-schema/doc-empty-section.err
> index 5f03a6d733f..711a0d629c2 100644
> --- a/tests/qapi-schema/doc-empty-section.err
> +++ b/tests/qapi-schema/doc-empty-section.err
> @@ -1 +1 @@
> -doc-empty-section.json:6: text required after 'Note:'
> +doc-empty-section.json:6: text required after 'Errors:'
> diff --git a/tests/qapi-schema/doc-empty-section.json 
> b/tests/qapi-schema/doc-empty-section.json
> index f3384e9a3bb..f179d3eff6d 100644
> --- a/tests/qapi-schema/doc-empty-section.json
> +++ b/tests/qapi-schema/doc-empty-section.json
> @@ -3,6 +3,6 @@
>  ##
>  # @foo:
>  #
> -# Note:
> +# Errors:
>  ##
>  { 'command': 'foo', 'data': {'a': 'int'} }
> diff --git a/tests/qapi-schema/doc-good.json b/tests/qapi-schema/doc-good.json
> index 8b39eb946af..4b338cc0186 100644
> --- a/tests/qapi-schema/doc-good.json
> +++ b/tests/qapi-schema/doc-good.json
> @@ -29,7 +29,7 @@
>  # - Second list
>  #   Note: still in list
>  #
> -# Note: not in list
> +# .. note:: not in list

Uh...  see [*] below.

>  #
>  # 1. Third list
>  #    is numbered
> @@ -157,7 +157,7 @@
>  # @cmd-feat1: a feature
>  # @cmd-feat2: another feature
>  #
> -# Note: @arg3 is undocumented
> +# .. note:: @arg3 is undocumented
>  #
>  # Returns: @Object
>  #
> @@ -165,7 +165,7 @@
>  #
>  # TODO: frobnicate
>  #
> -# Notes:
> +# .. admonition:: Notes
>  #
>  #  - Lorem ipsum dolor sit amet
>  #  - Ut enim ad minim veniam
> diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
> index 435f6e6d768..2c9b4e419cb 100644
> --- a/tests/qapi-schema/doc-good.out
> +++ b/tests/qapi-schema/doc-good.out
> @@ -76,7 +76,7 @@ Not in list
>  - Second list
>    Note: still in list
>  
> -Note: not in list

[*] This demonstrates the "Note: ..." is *not* recognized as a "Note"
section before the patch (compare to the change we get for recognized
sections below).

Obscure fact: the doc parser recognizes tagged sections *only* in
definition documentation.  Arguably a misfeature.

Your series makes the misfeature even more obscure, because afterwards,
the only tagged section left that could make sense in a free-form doc
comment is "TODO".  Let's not worry about the misfeature now.

Two sensible solutions:

1. Since the patch converts tagged sections, and this isn't, don't touch
it.  If you feel you want to mention this in the commit message, go
ahead.

2. Touch it anyway.  Do mention it in the commit message then.

> +.. note:: not in list
>  
>  1. Third list
>     is numbered
> @@ -169,15 +169,17 @@ description starts on the same line
>  a feature
>      feature=cmd-feat2
>  another feature
> -    section=Note
> -@arg3 is undocumented
> +    section=None
> +.. note:: @arg3 is undocumented
>      section=Returns
>  @Object
>      section=Errors
>  some
>      section=TODO
>  frobnicate
> -    section=Notes
> +    section=None
> +.. admonition:: Notes
> +
>   - Lorem ipsum dolor sit amet
>   - Ut enim ad minim veniam
>  
> diff --git a/tests/qapi-schema/doc-good.txt b/tests/qapi-schema/doc-good.txt
> index 847db70412d..b89f35d5476 100644
> --- a/tests/qapi-schema/doc-good.txt
> +++ b/tests/qapi-schema/doc-good.txt
> @@ -17,7 +17,9 @@ Not in list
>  
>  * Second list Note: still in list
>  
> -Note: not in list
> +Note:
> +
> +  not in list
>  
>  1. Third list is numbered
>  
> @@ -193,11 +195,9 @@ Features
>  "cmd-feat2"
>     another feature
>  
> +Note:
>  
> -Note
> -~~~~
> -
> -"arg3" is undocumented
> +  "arg3" is undocumented
>  
>  
>  Returns
> @@ -211,9 +211,7 @@ Errors
>  
>  some
>  
> -
> -Notes
> -~~~~~
> +Notes:
>  
>  * Lorem ipsum dolor sit amet
>  
> diff --git a/tests/qapi-schema/doc-interleaved-section.json 
> b/tests/qapi-schema/doc-interleaved-section.json
> index adb29e98daa..b26bc0bbb79 100644
> --- a/tests/qapi-schema/doc-interleaved-section.json
> +++ b/tests/qapi-schema/doc-interleaved-section.json
> @@ -10,7 +10,7 @@
>  #
>  #           bao
>  #
> -# Note: a section.
> +# Returns: a section.
>  #
>  # @foobar: catch this
>  #

"Returns:" is only valid for commands, and this is a struct.  Let's use
"TODO:" instead.