John Snow <js...@redhat.com> writes: > On Tue, Mar 25, 2025 at 5:42 AM Markus Armbruster <arm...@redhat.com> wrote: > >> John Snow <js...@redhat.com> writes: >> >> > The new qapidoc transmogrifier can generate "Returns" statements with >> > type information just fine, so we can remove it from the source where it >> > doesn't add anything particularly novel or helpful and just repeats the >> > type info. >> > >> > This patch does not touch Returns: lines that add some information >> > (potentially helpful, potentially not) but repeats the type information >> > to remove that type. >> > >> > Signed-off-by: John Snow <js...@redhat.com> >> >> This is a clear improvement for the generated docs. For instance, >> blockdev-snapshot-delete-internal-sync goes from >> >> Return: >> "SnapshotInfo" -- SnapshotInfo >> >> to >> >> Return: >> "SnapshotInfo" >> >> However, I see that *triplicated* in my testing. I observed similar >> issues with the previous patch, so let's discuss that there and ignore >> it here. >> >> The impact on schema file egonomics is less clear. >> >> This patch removes a bunch of "Returns:" sections that make the >> generated docs look bad. How can we stop people from writing such >> sections?
Plan A: catch it in review. If that turns out to be overly bothersome, we need to think about better tooling. >> Developers tend to refer to the schema file instead of the generated >> documentation. Information is spread across doc comment and schema >> code. Both describe the syntactic structure. Only the schema code has >> types, optional, and such. The doc comment describes semantics. In >> practice, skimming the doc comment is often enough. >> >> Except for the return value. The doc comment's "Returns:" section is >> optional. When it's absent, the generated docs are bad (but this patch >> fixes that). Moreover, the doc comment doesn't fully describe the >> syntactic structure then. Unwary readers may not be aware of that trap, >> and miss the return value. I've since pondered this some more, and also talked with John. When doc comments look like they provide a certain kind of information, but they are actually unrealiable, that's less than ideal for its readers. This has always been the case for member / argument descriptions. We didn't require them initially, and when we started to, things had gotten so bad that I had to provide an escape hatch: pragma documentation-exceptions still lists 44 offenders in qapi/ and one in qga/. Most of the offenders are doc bugs. But not all: documenting the members of QKeyCode one by one would be silly. It has also always been the case for return value descriptions. We still don't require them. Your series uncovered ten in qapi/ and one in qga/. Your series adds 46 in qapi/. Possibly more after review of the last patch. Missing Returns goes from rare to common. This does not create doc bugs. Generated documentation actually improves. I figure developers just have to mind that the doc comment need not be complete. >> The inliner you posted before needs to know where the inlined stuff >> goes. Obvious when there are argument descriptions or a "Returns:". >> For the cases where we have nothing useful, you proposed an explicit >> marker "Details:" (how exactly it's spelled doesn't matter here, only >> that an explicit marker can be necessary). Could removing "Returns:" >> make the marker necessary more often? Can our tooling reliably detect >> the need for the marker? >> > > Well, tooling can at least be certain when it isn't certain. > > The warning I have in my inliner branch-fork-whatever now basically just > looks at the sections and if there's non-plaintext sections between the > start and the ending, it treats the beginning as intro and the ending as > details. The non-plaintext sections are: Returns, Errors, Since, TODO. Returns and Errors are reliable anchors for the inliner. The inliner inlines argument sections. They need to go next to Returns / Errors sections, if any, because they get rendered together in as single two-column thing. Since feels useless as an anchor. Does the inline ignore it? I don't remember. Every definition doc should have it, and we commonly put it at the very end (currently 776 out of 984 times). When we don't, it's usually followed by examples only, and occasionally by notes. Putting it always last would be better. If we manage to replace handwritten by computed since information, Since goes away. TODO is the odd duck. It can go anywhere, which makes its use as anchor questionable. It's rare (I count 7 instances). One of them presses it into service to separate intro and example (commit 14b48aaab92). Your inliner series has a replacement for this hack; I believe the replacement can serve as a reliable anchor. > In the case there is *nothing else at all*, i.e. no returns, no > arguments/members, no errors, no features - i.e. it's a single QAPIDoc > Section - the inliner will count the *paragraphs*. If it's *one* paragraph, > it deduces that it's an intro section and does not consider it ambiguous. > If there are multiple paragraphs, however, that's when it emits a warning. This is a heuristic. We'll discuss it in review of the inliner. > A computer is never going to be able to reliably determine *intent*, but > syntactically I think that's a pretty narrow circumstance to yelp over: > "Documentation contains only a single plaintext section that consists of > two or more paragraphs". In practice, that's a reasonably rare occurrence > and is most likely to occur with query commands that take no arguments, ... or refer to a complex type for their arguments ... > have no features, and do not document return value semantics. ... and do not document errors. I'd be interested in the existing instances if you can track them down easily.
