qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: John Snow
Subject: Re: [PATCH 20/20] qapi: convert "Example" sections to rST
Date: Mon, 17 Jun 2024 17:27:16 -0400



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

>  #
>  #     -> { "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 ;)
 

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

(Also, this example doesn't use the QMP lexer, because it's not real QMP. It could be cajoled by making the lines string values, for example - or making it a more representative example that actually resembles QMP.)
 

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

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


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.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]