Re: [PATCH 20/20] qapi: convert "Example" sections to rST

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Fri, Jun 14, 2024 at 10:39 AM Markus Armbruster <armbru@redhat.com>
> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > Eliminate the "Example" sections in QAPI doc blocks, converting them
>> > into QMP example code blocks. This is generally done by converting
>> > "Example:" or "Examples:" lines into ".. code-block:: QMP" lines.
>> >
>> > This patch does also allow for the use of the rST syntax "Example::" by
>> > exempting double-colon syntax from the QAPI doc parser, but that form is
>> > not used by this conversion patch. The phrase "Example" here is not
>> > special, it is the double-colon syntax that transforms the following
>> > block into a code-block. By default, *this* form does not apply QMP
>> > highlighting.
>> >
>> > This patch has several benefits:
>> >
>> > 1. Example sections can now be written more arbitrarily, mixing
>> >    explanatory paragraphs and code blocks however desired.
>> >
>> > 2. Example sections can now use fully arbitrary rST.
>> >
>> > 3. All code blocks are now lexed and validated as QMP; increasing
>> >    usability of the docs and ensuring validity of example snippets.
>> >
>> > 4. Each code-block can be captioned independently without bypassing the
>> >    QMP lexer/validator.
>> >
>> > For any sections with more than one example, examples are split up into
>> > multiple code-block regions. If annotations are present, those
>> > annotations are converted into code-block captions instead, e.g.
>> >
>> > ```
>> > Examples:
>> >
>> >    1. Lorem Ipsum
>> >
>> >    -> { "foo": "bar" }
>> > ```
>> >
>> > Is rewritten as:
>> >
>> > ```
>> > .. code-block:: QMP
>> >    :caption: Example: Lorem Ipsum
>> >
>> >    -> { "foo": "bar" }
>> > ```
>> >
>> > This process was only semi-automated:
>> >
>> > 1. Replace "Examples?:" sections with sed:
>> >
>> > sed -i 's|# Example:|# .. code-block:: QMP|' *.json
>> > sed -i 's|# Examples:|# .. code-block:: QMP|' *.json
>> >
>> > 2. Identify sections that no longer parse successfully by attempting the
>> >    doc build, convert annotations into captions manually.
>> >    (Tedious, oh well.)
>> >
>> > 3. Add captions where still needed:
>> >
>> > sed -zi 's|# .. code-block:: QMP\n#\n|# .. code-block:: QMP\n# :caption: 
>> > Example\n#\n|g' *.json
>> >
>> > Not fully ideal, but hopefully not something that has to be done very
>> > often. (Or ever again.)
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>
>> > ---
>> >  qapi/acpi.json                  |   6 +-
>> >  qapi/block-core.json            | 120 ++++++++++++++++----------
>> >  qapi/block.json                 |  60 +++++++------
>> >  qapi/char.json                  |  36 ++++++--
>> >  qapi/control.json               |  16 ++--
>> >  qapi/dump.json                  |  12 ++-
>> >  qapi/machine-target.json        |   3 +-
>> >  qapi/machine.json               |  79 ++++++++++-------
>> >  qapi/migration.json             | 145 +++++++++++++++++++++++---------
>> >  qapi/misc-target.json           |  33 +++++---
>> >  qapi/misc.json                  |  48 +++++++----
>> >  qapi/net.json                   |  30 +++++--
>> >  qapi/pci.json                   |   6 +-
>> >  qapi/qapi-schema.json           |   6 +-
>> >  qapi/qdev.json                  |  15 +++-
>> >  qapi/qom.json                   |  20 +++--
>> >  qapi/replay.json                |  12 ++-
>> >  qapi/rocker.json                |  12 ++-
>> >  qapi/run-state.json             |  45 ++++++----
>> >  qapi/tpm.json                   |   9 +-
>> >  qapi/trace.json                 |   6 +-
>> >  qapi/transaction.json           |   3 +-
>> >  qapi/ui.json                    |  62 +++++++++-----
>> >  qapi/virtio.json                |  38 +++++----
>> >  qapi/yank.json                  |   6 +-
>> >  scripts/qapi/parser.py          |  15 +++-
>> >  tests/qapi-schema/doc-good.json |  12 +--
>> >  tests/qapi-schema/doc-good.out  |  17 ++--
>> >  tests/qapi-schema/doc-good.txt  |  17 +---
>> >  29 files changed, 574 insertions(+), 315 deletions(-)
>>
>> Missing: update of docs/devel/qapi-code-gen.rst.
>>
>> > diff --git a/qapi/acpi.json b/qapi/acpi.json
>> > index aa4dbe57943..3da01f1b7fc 100644
>> > --- a/qapi/acpi.json
>> > +++ b/qapi/acpi.json
>> > @@ -111,7 +111,8 @@
>> >  #
>> >  # Since: 2.1
>> >  #
>> > -# Example:
>> > +# .. code-block:: QMP
>> > +#    :caption: Example
>>
>> I wish this was a bit less verbose.  Oh well, we'll live.
>>
>
> We can create a custom directive that assumes a default caption; e.g.
>
> .. qapi:example::
>
>     ... blah blah blah ...
>
> And if you want to override it, you'd just
>
> .. qapi:example::
>     :caption: Lorem Ipsum ...
>
>     ... blah blah blah ...
>
> Interested? (I kept this patch vanilla to avoid getting fancy, but there
> are fanciness options available if you'd like them.)

Let's keep it simple for now.

>> >  #
>> >  #     -> { "execute": "query-acpi-ospm-status" }
>> >  #     <- { "return": [ { "device": "d1", "slot": "0", "slot-type": 
>> > "DIMM", "source": 1, "status": 0},
>>
>> This is rendered as a light green box with the caption on top, in
>> italics and centered.  I'm not sure I like the use of the caption.  The
>> previous patch's Note boxes look nicer.
>>
>
> We can change that with styling - my dedicated CSS intern was busy with
> finals when I wrote this patch ;)

Tell her I asked for another helping of her magic!

>> The contents of the box is highlighted.  I am sure I like that.
>>
>
> Yes.
>
> [...]
>
>> > -# Example:
>> > -#
>> > -#     Set new histograms for all io types with intervals
>> > -#     [0, 10), [10, 50), [50, 100), [100, +inf):
>> > +# .. code-block:: QMP
>> > +#    :caption: Example:
>> > +#      Set new histograms for all io types with intervals
>> > +#      [0, 10), [10, 50), [50, 100), [100, +inf):
>>
>> Captions long enough to be rendered as multiple lines look particularly
>> bad to me.  The centering...
>>
>
> Will attempt to address it with CSS. I do agree, just wasn't time to hammer
> it out.
>
> [...]
>
>
>> > @@ -134,7 +136,8 @@
>> >  #
>> >  # Since: 0.14
>> >  #
>> > -# Example:
>> > +# .. code-block:: QMP
>> > +#    :caption: Example
>> >  #
>> >  #     -> { "execute": "query-commands" }
>> >  #     <- {
>> > @@ -149,8 +152,8 @@
>> >  #          ]
>> >  #        }
>> >  #
>> > -#     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.
>>
>> Squash into the previous patch?
>>
>
> OK
>
> [...]
>
>
>> > diff --git a/qapi/pci.json b/qapi/pci.json
>> > index f51159a2c4c..9192212661b 100644
>> > --- a/qapi/pci.json
>> > +++ b/qapi/pci.json
>> > @@ -182,7 +182,8 @@
>> >  #
>> >  # Since: 0.14
>> >  #
>> > -# Example:
>> > +# .. code-block:: QMP
>> > +#    :caption: Example
>> >  #
>> >  #     -> { "execute": "query-pci" }
>> >  #     <- { "return": [
>> > @@ -311,8 +312,7 @@
>> >  #           ]
>> >  #        }
>> >  #
>> > -#     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.
>>
>> Squash into the previous patch?
>>
>
> OK
>
>
>>
>> >  #
>> >  ##
>> >  { 'command': 'query-pci', 'returns': ['PciInfo'] }
>> > diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
>> > index 5e33da7228f..66fbcbd3619 100644
>> > --- a/qapi/qapi-schema.json
>> > +++ b/qapi/qapi-schema.json
>> > @@ -20,11 +20,7 @@
>> >  # understand.  However, in real protocol usage, they're emitted as a
>> >  # single line.
>> >  #
>> > -# Also, the following notation is used to denote data flow:
>> > -#
>> > -# Example:
>> > -#
>> > -# ::
>> > +# Also, the following notation is used to denote data flow::
>> >  #
>> >  #   -> data issued by the Client
>> >  #   <- Server data response
>>
>> No use of caption here.  Looks better, I think.
>>
>
> OK - Let me play around with the styling, because I do want to have some
> kind of form option available for cargo-culting to add captions or an
> explanation of some kind. If I can't make it look good with CSS, I'll
> capitulate and mark them up as alternating normal paragraphs and examples.
>
> Forbidding "Examples?:" was just an easy way to make sure I converted
> everything; and especially to catch any late merges ... I am hesitant to go
> that route for maintainability. But, if you want to volunteer to play
> whack-a-mole for the next few releases, then...

Making use of the old tag a hard error is a smart move.  But I'm
prepared to sacrifice it for more nicely formatted examples.

> (Also, this example doesn't use the QMP lexer, because it's not real QMP.

Yes.

> It could be cajoled by making the lines string values, for example - or
> making it a more representative example that actually resembles QMP.)

No need unless it actually improves the generated docs.

>> > diff --git a/qapi/qdev.json b/qapi/qdev.json
>> > index d031fc3590d..cfe403fea20 100644
>> > --- a/qapi/qdev.json
>> > +++ b/qapi/qdev.json
>> > @@ -62,7 +62,8 @@
>> >  #        the ``-device DEVICE,help`` command-line argument, where DEVICE
>> >  #        is the device's name.
>> >  #
>> > -# Example:
>> > +# .. code-block:: QMP
>> > +#    :caption: Example
>> >  #
>> >  #     -> { "execute": "device_add",
>> >  #          "arguments": { "driver": "e1000", "id": "net1",
>>
>> How does
>>
>>    # Example:
>>   +# .. code-block:: QMP
>>    #
>>    #     -> { "execute": "device_add",
>>    #          "arguments": { "driver": "e1000", "id": "net1",
>>
>> look?  Requires nerfing the error you add to parser.py.
>>
>
> Undesirable, IMO -- but "Example::" alongside an option to choose the QMP
> lexer by default for QMP docs may be acceptable. I can demo some choices
> for you on a screenshare call if you'd like to workshop this aesthetic
> choice out together.
>
> [...]
>
>
>> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
>> > index 8b1da96124e..afc0b444034 100644
>> > --- a/scripts/qapi/parser.py
>> > +++ b/scripts/qapi/parser.py
>> > @@ -554,9 +554,12 @@ def get_doc(self) -> 'QAPIDoc':
>> >                      no_more_args = True
>> >                      intro = False
>> >                  elif match := re.match(
>> > -                        r'(Returns|Errors|Since|Notes?|Examples?|TODO): 
>> > *',
>> > +                        
>> > r'(Returns|Errors|Since|Notes?|Examples?(?!::)|TODO)'
>> > +                        r': *',
>> >                          line):
>>
>> Hmm, I wonder...
>>
>>
>> https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#literal-blocks
>> has:
>>
>>     Literal code blocks (ref) are introduced by ending a paragraph with
>>     the special marker ::.
>>
>> Not capturing regular rST markup like
>>
>>     Example::
>>
>>         mumble mumble
>>
>> for our own purposes makes sense.  But it makes exactly as much sense
>> for any of the tags, doesn'it?
>>
>> Should we instead change the regexp to match only when there's a
>> *single* colon?
>>
>
> OK. My regexp-fu is maybe weak, but I think I can just put (?!::): at the
> end of this regex without tying it to Examples, and I'll move that into its
> own patch.
>
>>
>>
>> > -                    # tagged section
>> > +                    # tagged section.
>>
>> Spurious comment change.
>>
>
> A *beautiful* comment change. An *inspired* comment change.
>
> (OK, removing it...)
>
>
>>
>> > +                    # Examples sections followed by two colons are 
>> > excluded;
>> > +                    # those are raw rST syntax!
>> >
>> >                      if 'Note' in match.group(1):
>> >                          emsg = (
>> > @@ -566,6 +569,14 @@ def get_doc(self) -> 'QAPIDoc':
>> >                          )
>> >                          raise QAPIParseError(self, emsg)
>> >
>> > +                    if match.group(1).startswith("Example"):
>> > +                        emsg = (
>> > +                            f"The '{match.group(1)}' section is 
>> > deprecated. "
>> > +                            "Please use rST's '.. code-block:: QMP' 
>> > directive,"
>> > +                            " 'Example::', or other suitable markup 
>> > instead."
>> > +                        )
>> > +                        raise QAPIParseError(self, emsg)
>> > +
>>
>> I guess this will be helpful while people get used to the changed
>> syntax.  Once they are, I'd like to get rid of it.  Same for "Note"
>> right above.
>>
>
> Yeah - the thinking was that it would help buffer the transitional period
> and could be removed after a release or two. I'll update the phrasing to
> not use "deprecated", also.

Throw in a TODO comment to remind us.

>> >                      doc.new_tagged_section(self.info, match.group(1))
>> >                      text = line[match.end():]
>> >                      if text:
>> > diff --git a/tests/qapi-schema/doc-good.json
>> b/tests/qapi-schema/doc-good.json
>> > index 0a294eb324e..57e2e591938 100644
>> > --- a/tests/qapi-schema/doc-good.json
>> > +++ b/tests/qapi-schema/doc-good.json
>> > @@ -46,11 +46,12 @@
>> >  #
>> >  # Duis aute irure dolor
>> >  #
>> > -# Example:
>> > +# .. code-block:: QMP
>>
>> No captions here?
>>
>
> They aren't *required*, I just liked having a dedicated place to put 'em in
> the rendered output for our real docs.

If captions are optional, doc-good should have at least one with
caption, and one without caption.

> [...]
>
>
>> I want this just as much as the previous patch.
>>
>>
> okie-dokey, I'll include it in the mini-fork of the pre-req series.