On Mon, Jun 16, 2025 at 7:36 AM Markus Armbruster <arm...@redhat.com> wrote:
> John Snow <js...@redhat.com> writes: > > > If we remove the legacy parser, the doc-good.json formatting begins to > > "parser"? You mean docs/sphinx/qapidoc_legacy.py, don't you? > Mmm... yes, I'm conflating the purpose of the series (removing the legacy freeform doc parser) with what necessitated this change (switching to the new doc *generator*) Wiggly-brained, wiggly-mouthed. > > > fail because the body text is appended directly after the field list > > entry, which is invalid rST syntax. > > We've been running the test suite with the legacy doc generator. > Unwise; we should've switched to the new one right away. > Oops O:-) > > > Without this change, we see this error: > > > > /home/jsnow/src/qemu/docs/../tests/qapi-schema/doc-good.json:169: > > WARNING: Field list ends without a blank line; unexpected > > unindent. [docutils] > > The reporting is less than helpful. > > > And this intermediate rST source: > > > > tests/qapi-schema/doc-good.json:0167 | :error: > > tests/qapi-schema/doc-good.json:0168 | some > > > > With this patch applied, we instead generate this source: > > > > tests/qapi-schema/doc-good.json:0167 | :error: > > tests/qapi-schema/doc-good.json:0168 | - some > > > > which compiles successfully. > > Hmm. > > As far as I can tell, the problem is lack of indentation[*]. > > By convention, the contents of an Errors: section is a list. > docs/devel/qapi-code-gen.rst: > > "Errors" sections should be formatted as an rST list, each entry > detailing a relevant error condition. For example:: > > # Errors: > # - If @device does not exist, DeviceNotFound > # - Any other error returns a GenericError. > > This test case is the only instance of something else. > > It's just a convention, though. > > Your change to the positive test case makes some sense all the same; it > should cover how we want the thing to be used. > > What I don't like is how the new doc generator fails when we fail to > adhere to the convention. > I agree... it should probably generate ":error: some" instead, without the newline. Bad, but valid. > > Here's docs/devel/qapi-code-gen.rst on tagged sections: > > A tagged section begins with a paragraph that starts with one of the > following words: "Since:", "Returns:", "Errors:", "TODO:". It ends > with > the start of a new section. > > The second and subsequent lines of tagged sections must be indented > like this:: > > # TODO: Ut enim ad minim veniam, quis nostrud exercitation ullamco > # laboris nisi ut aliquip ex ea commodo consequat. > # > # Duis aute irure dolor in reprehenderit in voluptate velit esse > # cillum dolore eu fugiat nulla pariatur. > > This tells us that > > # Errors: some > > and > > # Errors: > # some > > and > > # Errors: some > # more > > should all work, just like for any other tag. However, only the second > one works in my testing. With qapidoc_legacy.py, all three work. > > We can make Errors: unlike the other tags. But it needs to be done > properly, i.e. in scripts/qapi/parser.py (for decent error reporting), > and documented in docs/devel/qapi-code-gen.rst. > > Keeping the QAPI domain accept what the generator generates might be > easier. > > Thoughts? > I try not to have any as often as I can. In seriousness, I need a good few minutes with the generator to understand why this behaves strangely. Might need to switch to a different parser routine in parser.py, or I might need to adjust the qapidoc generator. Not entirely sure what's precisely wrong... If I can find something that's both easy and clean I'll do that instead. Probably a newline needs to be preserved in the source somewhere, and then a newline needs to be *not* added to the generator. Something like that. We can discuss turning our de-facto standard into an actual syntactical requirement later; I think there might be some benefit to allowing multiple errors sections that each become their own ":error:" info field list entry, but I am not confident on that and am not ready to dive into it just yet. I think it's definitely easier on the sphinx side but it might not be easier on the QAPI side. We'll see... > > > > > Signed-off-by: John Snow <js...@redhat.com> > > --- > > tests/qapi-schema/doc-good.json | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/tests/qapi-schema/doc-good.json > b/tests/qapi-schema/doc-good.json > > index 14b808f9090..6dcde8fd7e8 100644 > > --- a/tests/qapi-schema/doc-good.json > > +++ b/tests/qapi-schema/doc-good.json > > @@ -165,7 +165,8 @@ > > # > > # Returns: @Object > > # > > -# Errors: some > > +# Errors: > > +# - some > > # > > # TODO: frobnicate > > # > > Fails "make check". Fixup appended. > Strange. How'd I find this issue and fix it if I wasn't running the tests? Uhm, sorry. Sloppy of me. > > > > [*] Evidence: > > # Errors: > # - some > > which expands into > > :error: > - some > > and > > # Errors: > # some > > which expands into > > :error: > some > > both work. > > docs/devel/qapi-domain.rst: > > ``:error:`` > ----------- > > Document the error condition(s) of a QAPI command. > > :availability: This field list is only available in the body of the > Command directive. > --> :syntax: ``:error: Lorem ipsum dolor sit amet ...`` > :type: `sphinx.util.docfields.Field > < > https://pydoc.dev/sphinx/latest/sphinx.util.docfields.Field.html?private=1 > >`_ > > The format of the :errors: field list description is free-form rST. The > alternative spelling ":errors:" is also permitted, but strictly > analogous. > > Example:: > > .. qapi:command:: block-job-set-speed > :since: 1.1 > > Set maximum speed for a background block operation. > > This command can only be issued when there is an active block > job. > > Throttling can be disabled by setting the speed to 0. > > :arg string device: The job identifier. This used to be a device > name (hence the name of the parameter), but since QEMU 2.7 it > can have other values. > :arg int speed: the maximum speed, in bytes per second, or 0 for > unlimited. Defaults to 0. > --> :error: > --> - If no background operation is active on this device, > --> DeviceNotActive > > This makes me expect > > :error: some > > also works. However, the obvious > > # Errors: some > > produces > > :error: > some > > which doesn't work. > > > diff --git a/tests/qapi-schema/doc-good.out > b/tests/qapi-schema/doc-good.out > index dc8352eed4..3711cf5480 100644 > --- a/tests/qapi-schema/doc-good.out > +++ b/tests/qapi-schema/doc-good.out > @@ -176,7 +176,7 @@ another feature > section=Returns > @Object > section=Errors > -some > + - some > section=Todo > frobnicate > section=Plain > diff --git a/tests/qapi-schema/doc-good.txt > b/tests/qapi-schema/doc-good.txt > index 17a1d56ef1..e54cc95f4a 100644 > --- a/tests/qapi-schema/doc-good.txt > +++ b/tests/qapi-schema/doc-good.txt > @@ -207,7 +207,7 @@ Returns > Errors > ~~~~~~ > > -some > +* some > > Notes: > > >
