Re: [PATCH v6 07/21] docs/sphinx: Add new qapi-doc Sphinx extension

[Peter Maydell]

On Tue, 29 Sep 2020 at 07:54, Markus Armbruster <armbru@redhat.com> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > Some of our documentation is auto-generated from documentation
> > comments in the JSON schema.
> >
> > For Sphinx, rather than creating a file to include, the most natural
> > way to handle this is to have a small custom Sphinx extension which
> > processes the JSON file and inserts documentation into the rST
> > file being processed.
> >
> > This is the same approach that kerneldoc and hxtool use.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> Down to a few trivial things, which I can tidy up in my tree.

Thanks, that would be great.


> > +    def _serror(self, msg):
> > +        """Raise an exception giving a user-friendly syntax error 
> > message"""
> > +        file = self._cur_doc.info.fname
> > +        line = self._cur_doc.info.line
> > +        raise ExtensionError(
> > +            '%s line %d: syntax error: %s' % (file, line, msg))
>
> Unused, let's drop.

Oops, yes. I refactored the error-handling code below and forgot
to delete this now-unused method.

> > +    def visit_enum_type(self, name, info, ifcond, features, members, 
> > prefix):
> > +        doc = self._cur_doc
> > +        self._add_doc('Enum',
> > +                      self._nodes_for_enum_values(doc) +
> > +                      self._nodes_for_features(doc) +
> > +                      self._nodes_for_sections(doc) +
> > +                      self._nodes_for_if_section(ifcond))
>
> PEP 8: In Python code, it is permissible to break before or after a
> binary operator, as long as the convention is consistent locally.  For
> new code Knuth's style is suggested.
>
> Mind switching to Knuth style, i.e. break before the operator?

Fine with me, I have no strong Python style preferences.
I'm unlikely to be able to reliably follow any particular
style rules in future code I write unless there's a linter
that complains about violations, though...

> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 5eed1e692b4..dbddb0a7635 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -3149,6 +3149,7 @@ M: Peter Maydell <peter.maydell@linaro.org>
> >  S: Maintained
> >  F: docs/conf.py
> >  F: docs/*/conf.py
> > +F: docs/sphinx/
> >
> >  Miscellaneous
> >  -------------
>
> Maintainers of scripts/qapi/ should help review patches to
> docs/sphinx/qapidoc.py.  Two options:
>
> * Add docs/sphinx/qapidoc.py to section QAPI.  The QAPI maintainers
>   become M: of this small part of "Sphinx documentation configuration
>   and build machinery".  R: would be more accurate.  The inaccuracy
>   feels harmless.
>
> * Do nothing.  scripts/get_maintainer.pl won't loop in the QAPI
>   maintainers.  The Sphinx documentation maintainers may have to do that
>   manually.
>
> What do you think?

No strong preference. I guess putting qapidoc.py in the QAPI
section means that the right people get cc'd so that's the
better approach.

> My review is of uneven value: the QAPI-facing parts are good, the
> Sphinx-related parts merely look good to ignorant me.  I guess that's
> good enough, since we also have "generated docs look sane".
>
> Preferably tidied up a bit more:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

thanks
-- PMM