Re: [PATCH v5 09/20] docs/sphinx: Add new qapi-doc Sphinx extension

[Peter Maydell]

On Tue, 22 Sep 2020 at 12:42, Markus Armbruster <armbru@redhat.com> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Fri, 4 Sep 2020 at 13:29, Markus Armbruster <armbru@redhat.com> wrote:
> >> Got a pointer to "Writing Sphinx Extensions for Dummies" or similar?
> >
> > The upstream documentation on extending Sphinx is probably
> > a good place to start; in particular the tutorials are useful:
> >
> > https://www.sphinx-doc.org/en/master/development/index.html
>
> Do you think having this link in a comment could help future readers /
> maintainers?

Easy enough to add it, and it doesn't hurt.

> > OK, how about:
> >
> > # The purpose of this extension is to read the documentation comments
> > # in QAPI schema files, and insert them all into the current document.
> > #
> > # It implements one new rST directive, "qapi-doc::".
> > # Each qapi-doc:: directive takes one argument, which is the
> > # path of the .json file to process, relative to the source tree.
> > #
> > # The docs/conf.py file must set the qapidoc_srctree config value to
> > # the root of the QEMU source tree.
> >
> > ?
>
> Works for me with "pathname" instead of "path".  I'd also write "schema
> file" instead of ".json file", but that's a matter of taste and up to
> you.

Fixed.

> The doc string is confusing (I figure you borrowed the it along with the
> code).  str.join() works just fine for arrays.  It's not the kind of
> iterable that's the problem, it's the kind of thing the iterable yields:
> str.join() wants str, we have. docutils.node.literal.
>
> The lazy solution is to delete the confusing doc string.
>
> Here's my attempt at a non-lazy solution:
>
>     """Yield the members of *iterable* interspersed with *separator*."""

Docstring updated to that text.

> >> I think I'd rather have a separate _node_for_ifcond().  Not a demand.
> >
> > I'm not sure what you have in mind here, so I'll leave it as it is :-)
>
> _nodes_for_sections() is used like this:
>
>         self._add_doc(WHAT,
>                       [self._nodes_for_WHAT(doc, ...),
>                        self._nodes_for_features(doc),
>                        self._nodes_for_sections(doc, ifcond)])
>
> ._add_doc()'s second argument is a list of nodes for sections.
>
> ._nodes_for_WHAT() computes the nodes specific to the kind of thing
> we're processing: enum, object, alternate, command, event.
>
> ._nodes_for_features() computes the nodes for the feature section.
>
> ._nodes_for_sections() computes both the nodes for additional sections
> and the nodes for the ifcond section.
>
> I'd compute the nodes for the ifcond section in their own function, to
> keep .nodes_for_sections() focused on just one purpose.

Oh, OK, that makes sense. The name "_nodes_for_ifcond" is already
taken, though, so I'll call it "_nodes_for_if_section".

> >> Makes me wonder why you build two levels in the first place.
> >
> > This is probably just me not being a very experienced Python
> > programmer. What's the syntax for passing _add_doc the single list
> > which is just the concatenation of the various lists that
> > the _nodes_for_* methods return ?
>
> You could replace
>
>                       [self._nodes_for_WHAT(doc, ...),
>                        self._nodes_for_features(doc),
>                        self._nodes_for_sections(doc, ifcond)])
>
> by
>
>                       self._nodes_for_WHAT(doc, ...)
>                       + self._nodes_for_features(doc)
>                       + self._nodes_for_sections(doc, ifcond)
>
> Operator + concatenates sequences.

Right. We also need to change some of the _nodes_for_whatever
functions that were sometimes returning just a single node
(eg "return section") to instead return a one-element list
("return [section]"), and make them return [] where they were
previously returning None. But this seems like a good change,
as then all these functions consistently return a list
(which might have 0, 1 or many elements).

> > How do I get a QAPIError from within one of the
> > visit_* or freeform methods of a QAPISchemaVisitor ?
>
> The visit_ methods take an info argument.  With that:
>
>     raise QAPISemError(info, "the error message")

We need to launder it into an ExtensionError, so
   qapierr = QAPISemError(info, "message")
   raise ExtensionError(str(qapierr))

but otherwise this works nicely, and you get:

Extension error:
In file included from
/home/petmay01/linaro/qemu-from-laptop/qemu/docs/../qapi/qapi-schema.json:72:
/home/petmay01/linaro/qemu-from-laptop/qemu/docs/../qapi/block.json:4:
section or subsection heading must be in its own doc comment block

(Or we could do the launder-QAPIError-into-ExtensionError
in run() by extending the scope of its try..except, as I think
you suggest below.)

> > (Aside: what was your conclusion on the question of section
> > headers, anyway? I need to go back and re-read the older
> > threads but I think that is still an unresolved topic...)
>
> If I remember correctly, I disliked "[PATCH 19/29]
> qapi/qapi-schema.json: Put headers in their own doc-comment blocks",
> because "it sweeps the actual problem under the rug, namely flaws in our
> parsing and representation of doc comments."
>
> Our documentation has a tree structure.  Ideally, we'd represent it as a
> tree.  I figure such a tree would suit you well: you could translate
> more or less 1:1 to Sphinx nodes.
>
> We currently have a two-level list instead: a list of blocks (either
> definition or free-form), where each block has a list of sections (see
> QAPIDoc's doc string).
>
> My commit dcdc07a97c "qapi: Make section headings start a new doc
> comment block" was a minimally invasive patch to get us closer to a
> (partially flattened) tree.  Kind of collects the dirt in an (hopefully
> obvious) pile next to the rug.

Ah, yes, I'd forgotten that commit. So I can drop the bit of
code you disliked previously that insists on section headings
being in their own freeform block (handling "header line followed
by freeform text" is easy). That also incidentally means we're down
to only one parser-error in the qapidoc.py, which is the one that
catches mis-ordered section headings (eg using '=== header' when
you hadn't previously used '== header').

> You could finish the job and rework the representation into a tree.
> That would be lovely.  I appreciate help with cleaning up existing QAPI
> messes very much, but do not feel obliged.
>
> You could work with the two-level list as is, relying on the fact that a
> '=' can only occur right at the start of a free-form block.

I'll take this option, I think.

> You could tweak the two-level list just a bit, so that the heading
> becomes properly represented, and you don't have to match free-form body
> section text to find it.

thanks
-- PMM