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

[John Snow <jsnow@redhat.com>]

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

On Wed, Jun 19, 2024, 8:49 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.
> >
> > 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.
>

Right, it's gonna go away or be heavily reduced. A fish for another fry.


> 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.
>

Oh, uh. Alright. I wonder why I changed it then. I thought I was playing
error message whackamole with this one, but maybe I did do a regexp at some
point.

I'll leave it be if I can. If I can't, for some reason, then ...


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

... This, with why I couldn't leave it be.


> > +.. 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.
>

Weird that it didn't prohibit it. Bug?

--js

>

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