Re: [PATCH 31/57] qapi: expand tags to all doc sections

[John Snow <jsnow@redhat.com>]

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

On Wed, Mar 5, 2025 at 5:16 AM Markus Armbruster <armbru@redhat.com> wrote:

> Replaying review of a previous posting for your convenience...
>
> John Snow <jsnow@redhat.com> writes:
>
> > This patch adds an explicit section "kind" to all QAPIDoc
> > sections. Members/Features are now explicitly marked as such, with the
> > name now being stored in a dedicated "name" field (which qapidoc.py was
> > not actually using anyway.)
>
> I'm not sure what the parenthesis is trying to convey.
>

The old qapidoc.py doesn't actually use the name field, so there's nothing
to adjust for old callers.


>
> Before the patch, we have:
>
>               type        tag
>     untagged  Section     None
>     @foo:     ArgSection  'foo'
>     Returns:  Section     'Returns'
>     Errors:   Section     'Errors'
>     Since:    Section     'Since'
>     TODO:     Section     'TODO'
>
> Afterwards, I believe:
>
>               type         kind     name
>     untagged  Section      PLAIN
>     @foo:     ArgSection   MEMBER   'foo'   if member or argument
>               ArgSection   FEATURE  'foo'   if feature
>     Returns:  Section      RETURNS
>     Errors:   Section      ERRORS
>     Since:    Section      SINCE
>     TODO:     Section      TODO
>
> So, .tag is replaced by .kind and .name, member vs. feature vs. other
> tags is now obvious from .kind alone, i.e. there's no need to account
> for context or type.
>
> Fine print: why do we need to account for type before the patch?
> Consider @Since: ...
>

I'm not sure I follow...


>
> > The qapi-schema tests are updated to account for the new section names;
> > mostly "TODO" becomes "Todo" and `None` becomes "Plain".
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  docs/sphinx/qapidoc.py         |   7 ++-
> >  scripts/qapi/parser.py         | 109 ++++++++++++++++++++++++---------
> >  tests/qapi-schema/doc-good.out |  10 +--
> >  tests/qapi-schema/test-qapi.py |   2 +-
> >  4 files changed, 90 insertions(+), 38 deletions(-)
> >
> > diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> > index 61997fd21af..d622398f1da 100644
> > --- a/docs/sphinx/qapidoc.py
> > +++ b/docs/sphinx/qapidoc.py
> > @@ -35,6 +35,7 @@
> >  from docutils.statemachine import ViewList
> >  from qapi.error import QAPIError, QAPISemError
> >  from qapi.gen import QAPISchemaVisitor
> > +from qapi.parser import QAPIDoc
> >  from qapi.schema import QAPISchema
> >
> >  from sphinx import addnodes
> > @@ -258,11 +259,11 @@ def _nodes_for_sections(self, doc):
> >          """Return list of doctree nodes for additional sections"""
> >          nodelist = []
> >          for section in doc.sections:
> > -            if section.tag and section.tag == 'TODO':
> > +            if section.kind == QAPIDoc.Kind.TODO:
> >                  # Hide TODO: sections
> >                  continue
> >
> > -            if not section.tag:
> > +            if section.kind == QAPIDoc.Kind.PLAIN:
> >                  # Sphinx cannot handle sectionless titles;
> >                  # Instead, just append the results to the prior section.
> >                  container = nodes.container()
> > @@ -270,7 +271,7 @@ def _nodes_for_sections(self, doc):
> >                  nodelist += container.children
> >                  continue
> >
> > -            snode = self._make_section(section.tag)
> > +            snode = self._make_section(section.kind.name.title())
> >              self._parse_text_into_node(dedent(section.text), snode)
> >              nodelist.append(snode)
> >          return nodelist
> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> > index 36cb64a677a..c3004aa70c6 100644
> > --- a/scripts/qapi/parser.py
> > +++ b/scripts/qapi/parser.py
> > @@ -15,6 +15,7 @@
> >  # See the COPYING file in the top-level directory.
> >
> >  from collections import OrderedDict
> > +import enum
> >  import os
> >  import re
> >  from typing import (
> > @@ -575,7 +576,10 @@ def get_doc(self) -> 'QAPIDoc':
> >                          )
> >                          raise QAPIParseError(self, emsg)
> >
> > -                    doc.new_tagged_section(self.info, match.group(1))
> > +                    doc.new_tagged_section(
> > +                        self.info,
> > +                        QAPIDoc.Kind.from_string(match.group(1))
> > +                    )
> >                      text = line[match.end():]
> >                      if text:
> >                          doc.append_line(text)
> > @@ -586,7 +590,7 @@ def get_doc(self) -> 'QAPIDoc':
> >                          self,
> >                          "unexpected '=' markup in definition
> documentation")
> >                  else:
> > -                    # tag-less paragraph
> > +                    # plain paragraph(s)
>
> We're parsing a single pargraph here.  The plain section we add it to
> may have any number of paragraphs.  But for me, the comment is about
> what's being parsed.  Mind to drop (s)?
>

Anguish. I can't keep this straight in my head. OK. It wasn't obvious at a
glance where the break is if we get an empty newline ...


>
> >                      doc.ensure_untagged_section(self.info)
> >                      doc.append_line(line)
> >                      line = self.get_doc_paragraph(doc)
> > @@ -635,14 +639,37 @@ class QAPIDoc:
> >      Free-form documentation blocks consist only of a body section.
> >      """
> >
> > +    class Kind(enum.Enum):
> > +        PLAIN = 0
> > +        MEMBER = 1
> > +        FEATURE = 2
> > +        RETURNS = 3
> > +        ERRORS = 4
> > +        SINCE = 5
> > +        TODO = 6
> > +
> > +        @staticmethod
> > +        def from_string(kind: str) -> 'QAPIDoc.Kind':
>
> Remind me, why do we need to quote the type here?
>

It doesn't exist yet; it's a forward reference, basically. While we are in
the context of defining the class, we don't have access to variables scoped
within the class :)


>
> > +            return QAPIDoc.Kind[kind.upper()]
> > +
> > +        def text_required(self) -> bool:
> > +            # Only "plain" sections can be empty
> > +            return self.value not in (0,)
>
> Rather roundabout way to check for PLAIN, isn't it?
>

Vestigial from intro/details split. It can be removed here for now.


>
> There's just one caller (see below).  I doubt the method is worth its
> keep.
>

Vestigial again. Whether it's worth it once there are multiple such
sections, who knows. I thought it made the code read nicer in context. We
don't need it right now anyway...


>
> > +
> > +        def __str__(self) -> str:
> > +            return self.name.title()
> > +
>
> I wonder whether a simple StrEnum without methods would do.  Oh, StrEnum
> is new in 3.11.  Nevermind.
>
> Hmm.
>
>     >>> Kind = Enum('Kind', [('PLAIN', 'Plain'), ('TODO, 'TODO)])
>     >>> kind=Kind('Plain')
>     >>> kind.value
>     'Plain'
>
> What do you think?
>

Maybe, lemme play with it and see if it makes something else worse, I don't
know right away.


>
> >      class Section:
> >          # pylint: disable=too-few-public-methods
> > -        def __init__(self, info: QAPISourceInfo,
> > -                     tag: Optional[str] = None):
> > +        def __init__(
> > +            self,
> > +            info: QAPISourceInfo,
> > +            kind: 'QAPIDoc.Kind',
> > +        ):
> >              # section source info, i.e. where it begins
> >              self.info = info
> > -            # section tag, if any ('Returns', '@name', ...)
> > -            self.tag = tag
> > +            # section kind
> > +            self.kind = kind
> >              # section text without tag
> >              self.text = ''
> >
> > @@ -650,8 +677,14 @@ def append_line(self, line: str) -> None:
> >              self.text += line + '\n'
> >
> >      class ArgSection(Section):
> > -        def __init__(self, info: QAPISourceInfo, tag: str):
> > -            super().__init__(info, tag)
> > +        def __init__(
> > +            self,
> > +            info: QAPISourceInfo,
> > +            kind: 'QAPIDoc.Kind',
> > +            name: str
> > +        ):
> > +            super().__init__(info, kind)
> > +            self.name = name
> >              self.member: Optional['QAPISchemaMember'] = None
>
> Before the patch, use of a separate type for members, arguments and
> features was necessary to distinguish between '@TAG:' and 'TAG:' for the
> various TAGs.  This is no longer the case.  Fold ArgSection into
> Section?  Not sure.  If yes, separate patch to keep this one as
> mechanical as possible.
>

Possibly the case. I'll play with it. Do you want it in this series? (It's
pretty long as is...!)


>
> >
> >          def connect(self, member: 'QAPISchemaMember') -> None:
> > @@ -663,7 +696,9 @@ def __init__(self, info: QAPISourceInfo, symbol:
> Optional[str] = None):
> >          # definition doc's symbol, None for free-form doc
> >          self.symbol: Optional[str] = symbol
> >          # the sections in textual order
> > -        self.all_sections: List[QAPIDoc.Section] =
> [QAPIDoc.Section(info)]
> > +        self.all_sections: List[QAPIDoc.Section] = [
> > +            QAPIDoc.Section(info, QAPIDoc.Kind.PLAIN)
> > +        ]
> >          # the body section
> >          self.body: Optional[QAPIDoc.Section] = self.all_sections[0]
> >          # dicts mapping parameter/feature names to their description
> > @@ -680,12 +715,17 @@ def __init__(self, info: QAPISourceInfo, symbol:
> Optional[str] = None):
> >      def end(self) -> None:
> >          for section in self.all_sections:
> >              section.text = section.text.strip('\n')
> > -            if section.tag is not None and section.text == '':
> > +            if section.kind.text_required() and section.text == '':
>
> This is the only use of .text_required().  I believe checking for PLAIN
> would be clearer.
>
> >                  raise QAPISemError(
> > -                    section.info, "text required after '%s:'" %
> section.tag)
> > +                    section.info, "text required after '%s:'" %
> section.kind)
> >
> > -    def ensure_untagged_section(self, info: QAPISourceInfo) -> None:
> > -        if self.all_sections and not self.all_sections[-1].tag:
> > +    def ensure_untagged_section(
> > +        self,
> > +        info: QAPISourceInfo,
> > +    ) -> None:
>
> Accidental line breaking?
>

Something something something black autoformatter. I wonder why it did
this, though... I'll see if I can undo it.


>
> > +        kind = QAPIDoc.Kind.PLAIN
> > +
> > +        if self.all_sections and self.all_sections[-1].kind == kind:
>
> I'd prefer not to hide PLAIN behind a variable, but I'd also prefer
> the condition to fit on a line.  Hmm.
>

Also somewhat vestigial from when I had intro/details. When that split
comes, kind = becomes a conditional ternary.


>
> >              # extend current section
> >              section = self.all_sections[-1]
> >              if not section.text:
>
> Maybe
>
>            section = self.all_sections[-1] if self.all_sections else None
>
>            if second and section.kind = QAPIDoc.Kind.Plain:
>                # extend current section
>                if not section.text:
>

Could, but it's going to get rewritten when the inliner comes anyway, ...


>
> > @@ -693,46 +733,56 @@ def ensure_untagged_section(self, info:
> QAPISourceInfo) -> None:
> >                  section.info = info
> >              section.text += '\n'
> >              return
> > +
> >          # start new section
> > -        section = self.Section(info)
> > +        section = self.Section(info, kind)
> >          self.sections.append(section)
> >          self.all_sections.append(section)
> >
> > -    def new_tagged_section(self, info: QAPISourceInfo, tag: str) ->
> None:
> > -        section = self.Section(info, tag)
> > -        if tag == 'Returns':
> > +    def new_tagged_section(
> > +        self,
> > +        info: QAPISourceInfo,
> > +        kind: 'QAPIDoc.Kind',
> > +    ) -> None:
> > +        section = self.Section(info, kind)
> > +        if kind == QAPIDoc.Kind.RETURNS:
> >              if self.returns:
> >                  raise QAPISemError(
> > -                    info, "duplicated '%s' section" % tag)
> > +                    info, "duplicated '%s' section" % kind)
> >              self.returns = section
> > -        elif tag == 'Errors':
> > +        elif kind == QAPIDoc.Kind.ERRORS:
> >              if self.errors:
> >                  raise QAPISemError(
> > -                    info, "duplicated '%s' section" % tag)
> > +                    info, "duplicated '%s' section" % kind)
> >              self.errors = section
> > -        elif tag == 'Since':
> > +        elif kind == QAPIDoc.Kind.SINCE:
> >              if self.since:
> >                  raise QAPISemError(
> > -                    info, "duplicated '%s' section" % tag)
> > +                    info, "duplicated '%s' section" % kind)
> >              self.since = section
> >          self.sections.append(section)
> >          self.all_sections.append(section)
> >
> > -    def _new_description(self, info: QAPISourceInfo, name: str,
> > -                         desc: Dict[str, ArgSection]) -> None:
> > +    def _new_description(
> > +        self,
> > +        info: QAPISourceInfo,
> > +        name: str,
> > +        kind: 'QAPIDoc.Kind',
> > +        desc: Dict[str, ArgSection]
> > +    ) -> None:
> >          if not name:
> >              raise QAPISemError(info, "invalid parameter name")
> >          if name in desc:
> >              raise QAPISemError(info, "'%s' parameter name duplicated" %
> name)
> > -        section = self.ArgSection(info, '@' + name)
> > +        section = self.ArgSection(info, kind, name)
> >          self.all_sections.append(section)
> >          desc[name] = section
> >
> >      def new_argument(self, info: QAPISourceInfo, name: str) -> None:
> > -        self._new_description(info, name, self.args)
> > +        self._new_description(info, name, QAPIDoc.Kind.MEMBER,
> self.args)
> >
> >      def new_feature(self, info: QAPISourceInfo, name: str) -> None:
> > -        self._new_description(info, name, self.features)
> > +        self._new_description(info, name, QAPIDoc.Kind.FEATURE,
> self.features)
>
> QAPIDoc.Kind.FOO is a mouthful, and it tends to result in long lines,
> like here.  Can't see an easy and clean way to reduce the verbosity.
>

Yeah...


>
> >
> >      def append_line(self, line: str) -> None:
> >          self.all_sections[-1].append_line(line)
> > @@ -744,8 +794,9 @@ def connect_member(self, member: 'QAPISchemaMember')
> -> None:
> >                  raise QAPISemError(member.info,
> >                                     "%s '%s' lacks documentation"
> >                                     % (member.role, member.name))
> > -            self.args[member.name] = QAPIDoc.ArgSection(
> > -                self.info, '@' + member.name)
> > +            section = QAPIDoc.ArgSection(
> > +                self.info, QAPIDoc.Kind.MEMBER, member.name)
> > +            self.args[member.name] = section
>
> Why the extra variable?
>

Wish I remembered. I can undo it and see if anything barks.


>
> >          self.args[member.name].connect(member)
> >
> >      def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
> > diff --git a/tests/qapi-schema/doc-good.out
> b/tests/qapi-schema/doc-good.out
> > index 0a9da3efdeb..5773f1dd6d6 100644
> > --- a/tests/qapi-schema/doc-good.out
> > +++ b/tests/qapi-schema/doc-good.out
> > @@ -113,7 +113,7 @@ The _one_ {and only}, description on the same line
> >  Also _one_ {and only}
> >      feature=enum-member-feat
> >  a member feature
> > -    section=None
> > +    section=Plain
> >  @two is undocumented
> >  doc symbol=Base
> >      body=
> > @@ -171,15 +171,15 @@ description starts on the same line
> >  a feature
> >      feature=cmd-feat2
> >  another feature
> > -    section=None
> > +    section=Plain
> >  .. note:: @arg3 is undocumented
> >      section=Returns
> >  @Object
> >      section=Errors
> >  some
> > -    section=TODO
> > +    section=Todo
>
> With the method-less Enum I suggested, this hunk would go away.  Not
> that it matters :)
>
> >  frobnicate
> > -    section=None
> > +    section=Plain
> >  .. admonition:: Notes
> >
> >   - Lorem ipsum dolor sit amet
> > @@ -212,7 +212,7 @@ If you're bored enough to read this, go see a video
> of boxed cats
> >  a feature
> >      feature=cmd-feat2
> >  another feature
> > -    section=None
> > +    section=Plain
> >  .. qmp-example::
> >
> >     -> "this example"
> > diff --git a/tests/qapi-schema/test-qapi.py
> b/tests/qapi-schema/test-qapi.py
> > index 7e3f9f4aa1f..bca924309be 100755
> > --- a/tests/qapi-schema/test-qapi.py
> > +++ b/tests/qapi-schema/test-qapi.py
> > @@ -131,7 +131,7 @@ def test_frontend(fname):
> >          for feat, section in doc.features.items():
> >              print('    feature=%s\n%s' % (feat, section.text))
> >          for section in doc.sections:
> > -            print('    section=%s\n%s' % (section.tag, section.text))
> > +            print('    section=%s\n%s' % (section.kind, section.text))
> >
> >
> >  def open_test_result(dir_name, file_name, update):
>
>

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