On Wed, Mar 5, 2025 at 5:16 AM Markus Armbruster <arm...@redhat.com> wrote:
> Replaying review of a previous posting for your convenience...
>
> John Snow <js...@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 <js...@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):
>
>
