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>
---
docs/sphinx/qapidoc.py | 14 ++++++++------
scripts/qapi/parser.py | 11 +++++++++++
scripts/qapi/schema.py | 3 +++
3 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 0930307bc73..aaf9921c06c 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -255,16 +255,18 @@ def visit_feature(self, section: QAPIDoc.ArgSection) -> None:
def visit_returns(self, section: QAPIDoc.Section) -> None:
assert isinstance(self.entity, QAPISchemaCommand)
rtype = self.entity.ret_type
- # q_empty can produce None, but we won't be documenting anything
- # without an explicit return statement in the doc block, and we
- # should not have any such explicit statements when there is no
- # return value.
+ # return statements will not be present (and won't be
+ # autogenerated) for any command that doesn't return
+ # *something*, so ret_type will always be defined here.
assert rtype
typ = self.format_type(rtype)
assert typ
- assert section.text
- self.add_field("return", typ, section.text, section.info)
+
+ if section.text:
+ self.add_field("return", typ, section.text, section.info)
+ else:
+ self.add_lines(f":return-nodesc: {typ}", section.info)
def visit_errors(self, section: QAPIDoc.Section) -> None:
# FIXME: the formatting for errors may be inconsistent and may
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.
+ 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(
--
2.48.1John 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? Should we tighten section order some more? > + 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(
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.) > > > + 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( > >
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( >> >>
John Snow <jsnow@redhat.com> writes: > 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. ACK. Let's discuss this in review of v2. [...]
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>
A number of commands lack return value documentation before the patch.
These are:
QGA: guest-network-get-route
QMP: x-debug-query-block-graph, query-tpm, query-dirty-rate,
query-vcpu-dirty-limit, query-vm-generation-id,
query-memory-size-summary, query-memory-devices,
query-acpi-ospm-status, query-stats-schemas, query-stats-schemas
This patch fixes that. However, in my testing, it adds the missing
"Return:" doc *twice* for x-debug-query-block-graph and
query-dirty-rate.
On Tue, Mar 25, 2025 at 4:54 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> > > A number of commands lack return value documentation before the patch. > These are: > > QGA: guest-network-get-route > > QMP: x-debug-query-block-graph, query-tpm, query-dirty-rate, > query-vcpu-dirty-limit, query-vm-generation-id, > query-memory-size-summary, query-memory-devices, > query-acpi-ospm-status, query-stats-schemas, query-stats-schemas > > This patch fixes that. However, in my testing, it adds the missing > "Return:" doc *twice* for x-debug-query-block-graph and > query-dirty-rate. Guess who forgot a "break" statement? --js
© 2016 - 2025 Red Hat, Inc.