Re: [PATCH 2/4] docs, qapi: generate undocumented return sections

[John Snow <jsnow@redhat.com>]

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

On Tue, Mar 25, 2025 at 1:47 PM John Snow <jsnow@redhat.com> wrote:

>
>
> On Tue, Mar 25, 2025 at 5:41 AM Markus Armbruster <armbru@redhat.com>
> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > This patch changes the qapidoc transmogrifier to generate Return value
>> > documentation for any command that has a return value but hasn't
>> > explicitly documented that return value.
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>
>>
>> [...]
>>
>> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
>> > index 949d9e8bff7..8c382a049af 100644
>> > --- a/scripts/qapi/parser.py
>> > +++ b/scripts/qapi/parser.py
>> > @@ -815,6 +815,17 @@ def connect_feature(self, feature:
>> 'QAPISchemaFeature') -> None:
>> >                                 % feature.name)
>> >          self.features[feature.name].connect(feature)
>> >
>> > +    def ensure_returns(self, info: QAPISourceInfo) -> None:
>> > +        if not any(s.kind == QAPIDoc.Kind.RETURNS for s in
>> self.all_sections):
>> > +
>> > +            # Stub "Returns" section for undocumented returns value.
>> > +            # Insert stub after the last non-PLAIN section.
>>
>> Can you explain why that's where it should go?
>>
>
> ... No.
>
> (Joking...)
>
> I'm open to better positions if you can define them, admittedly I just
> picked a place that's likely to be at the end of the info field list
> sections. (Reminder: "info field list" means the sections that are
> converted directly into the two-column layout section of the rendered docs.)
>
>
>>
>> Should we tighten section order some more?
>>
>
> I wouldn't mind, but I believe this needs to be a change that you direct.
> From memory, I believe my preferred "enforced order" is something like this:
>
> 1. Intro paragraph(s)
> 2. Members
> 3. Features
> 4. Errors
> 5. Returns
> 6. Detail paragraph(s)
>
> ...Give or take some re-ordering between features/errors/returns as
> appropriate, I don't actually really care about the order there so much as
> I care about the fact that plain paragraphs do not appear between the
> members-features-errors-returns "region". The rest can be your preference.
>
> (Since and TODO can go wherever, from the perspective of the
> transmogrifier, I do not care about them since I do not render them in the
> document flow.)
>

With the insertions fixed to not duplicate/triplicate things, I notice
these (unintentional) changes:

- x-debug-block-dirty-bitmap-sha256 moves returns from above errors to below
- blockdev-snapshot-delete-internal-sync ditto
- query-xen-replication-status ditto
- query-colo-status ditto
- query-balloon ditto
- query-hv-balloon-status-report ditto
- query-yank -- this one actually moves it from above what would be details
to below -- this creates a new ambiguous case as we discussed on call
earlier today.
- add-fd goes from above errors to below errors again.



>
>
>>
>> > +            for sect in reversed(self.all_sections):
>> > +                if sect.kind != QAPIDoc.Kind.PLAIN:
>> > +                    stub = QAPIDoc.Section(info, QAPIDoc.Kind.RETURNS)
>> > +                    idx = self.all_sections.index(sect) + 1
>> > +                    self.all_sections.insert(idx, stub)
>> > +
>> >      def check_expr(self, expr: QAPIExpression) -> None:
>> >          if 'command' in expr:
>> >              if self.returns and 'returns' not in expr:
>> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
>> > index cbe3b5aa91e..3abddea3525 100644
>> > --- a/scripts/qapi/schema.py
>> > +++ b/scripts/qapi/schema.py
>> > @@ -1062,6 +1062,9 @@ def connect_doc(self, doc: Optional[QAPIDoc] =
>> None) -> None:
>> >              if self.arg_type and self.arg_type.is_implicit():
>> >                  self.arg_type.connect_doc(doc)
>> >
>> > +            if self.ret_type and self.info:
>> > +                doc.ensure_returns(self.info)
>> > +
>> >      def visit(self, visitor: QAPISchemaVisitor) -> None:
>> >          super().visit(visitor)
>> >          visitor.visit_command(
>>
>>

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