On Tue, Mar 25, 2025 at 1:47 PM John Snow <js...@redhat.com> wrote:
> > > On Tue, Mar 25, 2025 at 5:41 AM Markus Armbruster <arm...@redhat.com> > wrote: > >> John Snow <js...@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 <js...@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( >> >>
