John Snow <[email protected]> writes:
> On Tue, Sep 29, 2026, 8:36 AM Markus Armbruster <[email protected]> wrote:
>
>> John Snow <[email protected]> writes:
>>
>> > Enforce the relative ordering of "Intro", "Members", "Features",
>> > "Errors" and "Returns" sections.
>> >
>> > Signed-off-by: John Snow <[email protected]>
>> > ---
>> > docs/devel/qapi-code-gen.rst | 19 +++++++++++------
>> > scripts/qapi/parser.py | 41 ++++++++++++++++++++++++++++++------
>> > 2 files changed, 48 insertions(+), 12 deletions(-)
>> >
>> > diff --git a/docs/devel/qapi-code-gen.rst b/docs/devel/qapi-code-gen.rst
>> > index 3ac1c2dae18..29e9c5a3da6 100644
>> > --- a/docs/devel/qapi-code-gen.rst
>> > +++ b/docs/devel/qapi-code-gen.rst
>> > @@ -985,12 +985,19 @@ When documentation is required (see pragma_
>> 'doc-required'), every
>> > definition must have documentation.
>> >
>> > Definition documentation starts with a description naming the
>> > -definition with an optional indented overview (the "intro"),
>> > -:ref:`descriptions <doc-descrip>` of each argument (for commands and
>> > -events), member (for structs and unions), branch (for alternates), or
>> > -value (for enums), :ref:`feature descriptions <doc-feat>` of each
>> > -feature (if any), and finally optional :ref:`tagged sections
>> > -<doc-tagged>`.
>> > +definition with an optional indented overview (the "intro").
>> > +
>> > +The introduction is followed by :ref:`descriptions <doc-descrip>` of
>> > +each argument (for commands and events), member (for structs and
>> > +unions), branch (for alternates), or value (for enums).
>> > +
>> > +:ref:`Tagged sections <doc-tagged>` follow in order: Returns (for
>> > +commands), Errors (for commands), and lastly :ref:`feature
>> > +descriptions <doc-feat>` if applicable.
>>
>> *Some* of the tagged sections do.
>>
>> Does this document still need the concept "tagged section"?
>>
>
> I doubt it does, beyond an explanation of common syntax. Happy to hear
> suggestions...
I'll keep it in mind when I review PATCH 02.
> We could also stand to change some of the method names like "ensure
> untagged section" could become "ensure details", and the like.
Fair!
> tagged vs untagged has become increasingly non-meaningful.
True.
>> > +
>> > +Definition documentation concludes with rST-formatted detail
>> > +paragraphs (including examples, notes, &c), and finally an optional
>> > +"Since" tagged section at the very end.
>> >
>> > .. _doc-descrip:
>> >
>> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
>> > index fe284a3f866..94a60a23482 100644
>> > --- a/scripts/qapi/parser.py
>> > +++ b/scripts/qapi/parser.py
>> > @@ -545,6 +545,28 @@ def get_doc(self) -> 'QAPIDoc':
>> > doc.all_sections.append(QAPIDoc.Section(info,
>> QAPIDoc.Kind.INTRO))
>> > line = self.get_doc_indented(doc)
>> > no_more_args = False
>> > + last_ordered_section = QAPIDoc.Kind.INTRO
>> > +
>> > + def _tag_check(this: Union['QAPIDoc.Kind', str]) -> None:
>> > + nonlocal last_ordered_section
>> > + if isinstance(this, str):
>> > + this = QAPIDoc.Kind.from_string(this)
>>
>> Simpler, I think: require a .Kind argument, and do the conversion from
>> string in the one caller that needs it.
>>
>
> You're right.
>
>
>> > +
>> > + if this in (QAPIDoc.Kind.TODO, QAPIDoc.Kind.SINCE,
>> > + QAPIDoc.Kind.DETAILS):
>> > + return
>> > +
>> > + if this.value < last_ordered_section.value:
>> > + raise QAPIParseError(
>> > + self,
>> > + f"'{this}' section cannot appear after "
>> > + f"'{last_ordered_section}' section, please
>> re-order "
>> > + "the sections and adjust phrasing as necessary
>> to "
>> > + "ensure consistent documentation flow between
>> the "
>> > + "source code and the rendered HTML manual"
>> > + )
>>
>> That's a loooong error message. It actually consists of the error
>> proper and advice on how to correct it. Please use
>>
>> "<error>\n<advice>"
>>
>> like we do in expr.py's check_keys(), parser.py's
>> ._validate_doc_line_format(), and schema.py's ._def_definition(). Yes,
>> the embedded newline is a bit of a hack.
>>
>
> ACK.
>
>
>> > +
>> > + last_ordered_section = this
>> >
>> > while line is not None:
>> > # Blank lines
>> > @@ -558,6 +580,7 @@ def get_doc(self) -> 'QAPIDoc':
>> > if doc.has_features:
>> > raise QAPIParseError(
>> > self, "duplicated 'Features:' line")
>> > + _tag_check(QAPIDoc.Kind.FEATURE)
>> > self.accept(False)
>> > line = self.get_doc_line()
>> > while line == '':
>> > @@ -581,6 +604,7 @@ def get_doc(self) -> 'QAPIDoc':
>> > self,
>> > "description of '@%s:' follows a section"
>> > % match.group(1))
>> > + _tag_check(QAPIDoc.Kind.MEMBER)
>> > while (line is not None
>> > and (match :=
>> self._match_at_name_colon(line))):
>> > doc.new_argument(self.info, match.group(1))
>> > @@ -621,6 +645,7 @@ def get_doc(self) -> 'QAPIDoc':
>> > )
>> > raise QAPIParseError(self, emsg)
>> >
>> > + _tag_check(match.group(1))
>>
>> This is the caller that needs it.
>>
>
> Yep.
>
>
>> > doc.new_tagged_section(
>> > self.info,
>> > QAPIDoc.Kind.from_string(match.group(1))
>> > @@ -632,6 +657,7 @@ def get_doc(self) -> 'QAPIDoc':
>> > no_more_args = True
>> > else:
>> > # plain paragraph(s)
>> > + _tag_check(QAPIDoc.Kind.DETAILS)
>> > doc.ensure_untagged_section(self.info)
>> > doc.append_line(line)
>> > line = self.get_doc_paragraph(doc)
>> > @@ -674,14 +700,17 @@ class QAPIDoc:
>> > """
>> >
>> > class Kind(enum.Enum):
>> > - DETAILS = 0
>> > + # The order here is the order in which sections must appear in
>> > + # source code; with the exception of 'TODO' which may appear
>> > + # anywhere but is treated as a comment.
>> > + INTRO = 0
>> > MEMBER = 1
>> > - FEATURE = 2
>> > - RETURNS = 3
>> > - ERRORS = 4
>> > - SINCE = 5
>> > + RETURNS = 2
>> > + ERRORS = 3
>> > + FEATURE = 4
>> > + DETAILS = 5
>> > TODO = 6
>> > - INTRO = 7
>> > + SINCE = 7
>> >
>> > @staticmethod
>> > def from_string(kind: str) -> 'QAPIDoc.Kind':
>>
>> Missing: negative test for the new error.
>>
>
> Yep. If you are overall pleased with this change enough (pending nits),
> I'll add the tests now.
I am!
