Re: [PATCH 1/4] docs: lift block-core.json rST header into parents

[Kevin Wolf]

Am 09.09.2020 um 09:38 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 08.09.2020 um 14:03 hat Laszlo Ersek geschrieben:
> >> Hi Stefan,
> >> 
> >> On 09/08/20 11:31, Stefan Hajnoczi wrote:
> >> > block-core.json is included from several places. It has no way of
> >> > knowing what header level (h1, h2, ...) is appropriate. Sphinx reports
> >> > errors when it encounters an h2 header where it expects an h1 header.
> >> > This issue prevents the next patch from generating documentation for
> >> > qemu-storage-daemon QMP commands.
> >> > 
> >> > Move the header into parents so that the correct header level can be
> >> > used. Note that transaction.json is not updated since it doesn't seem to
> >> > need a header.
> >> > 
> >> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >> > ---
> >> >  docs/interop/firmware.json | 4 ++++
> >> >  qapi/block-core.json       | 4 ----
> >> >  qapi/block.json            | 1 +
> >> >  3 files changed, 5 insertions(+), 4 deletions(-)
> >> > 
> >> > diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
> >> > index 989f10b626..48af327f98 100644
> >> > --- a/docs/interop/firmware.json
> >> > +++ b/docs/interop/firmware.json
> >> > @@ -15,6 +15,10 @@
> >> >  ##
> >> >  
> >> >  { 'include' : 'machine.json' }
> >> > +
> >> > +##
> >> > +# == Block devices
> >> > +##
> >> >  { 'include' : 'block-core.json' }
> >> >  
> >> >  ##
> >> 
> >> I think "docs/interop/firmware.json" deserves the same treatment as
> >> "transaction.json".
> >> 
> >> It's been a long time since I last looked at a rendered view of
> >> "docs/interop/firmware.json", but it only includes "block-core.json" so
> >> it can refer to some block-related types (@BlockdevDriver seems like the
> >> main, or only, one).
> >> 
> >> I wouldn't expect the rendered view of "firmware.json" to have a section
> >> header saying "Block devices".
> >> 
> >> I think it should be fine to drop this hunk (and my CC along with it ;))
> >
> > I think this is actually a more general problem with the way we generate
> > the documentation. For example, the "Background jobs" documentation ends
> > up under "Block Devices" just because qapi-schema.json includes
> > block-core.json before job.json and block-core.json includes job.json to
> > be able to access some types.
> 
> The doc generator is stupid and greedy (which also makes it
> predictable): a module's documentation is emitted where it is first
> included.
> 
> For full control of the order, have the main module include all
> sub-modules in the order you want.

This works as long as the order that we want is consistent with the
requirement that every file must be included by qapi-schea.json before
it is included by any other file (essentially making the additional
includes in other files useless).

Is this the order that we want?

If so, we could support following the rule consistently by making double
include of a file an error.

> Alternatively, add just enough includes to get the order you want.

There are orders that I can't get this way. For example, if I want to
have "Block devices" documented before "Background jobs", there is no
way to add includes to actually get this order without having
"Background jobs" as a subsection in "Block devices".

> > Maybe we should always look for the least nested include directive to
> > figure out where the documentation should be placed. Then things
> > directly referenced by qapi-schema.json would always be on the top
> > level.
> >
> > Possibly we would then want to remove some includes from
> > qapi-schema.json and include them only from some other file to group
> > documentation sections that actually make sense to be grouped together.
> 
> I doubt implementing this feature would pay back the invested time.
> Manually controlling the order like I described above is not much of a
> burden, isn't it?

Depends on whether we are okay with the limitations of the tool
dictating the order of sections in our documentation.

Kevin