Re: [PATCH 00/42] docs: add sphinx-domain rST generator to qapidoc

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> "The text handler you add looks just like the existing latex handler. Does
> LaTeX output lack "little headings", too?"
>
> Yes, almost certainly. Can you let me know which output formats we actually
> "care about"? I'll have to test them all.

As far as I can tell, our build system runs sphinx-build -b html and -b
man.

I run it with -b text manually all the time to hunt for and review
changes in output.  I'd prefer to keep it working if practical.

For what it's worth, there is a bit of LaTeX configuration in
docs/conf.py.

>                                           In the meantime, I upgraded my
> patch so that the text translator properly handles branches with headings
> that delineate the different branches so that the text output is fully
> reasonable. I will need to do the same for any format we care about.
>
> I've re-pushed as of "about 30 minutes before I wrote this email" --
> https://gitlab.com/jsnow/qemu/-/commits/sphinx-domain-blergh2
>
> This branch includes the text generator fixes (which technically belong
> with the predecessor series we skipped, but I'll refactor that later.)
> it also includes fixes to the branch inliner, generated return statements,
> and generated out-of-band feature sections.

I'll fetch it, thanks!

> (Long story short: inserting new sections in certain spots was broken
> because of cache. Oops. We can discuss more why I wrote that part of the
> code like I did in review for the patch that introduced that problem. It's
> the "basic inliner" patch.)
>
> Below, I'm going to try a new communication approach where I explicitly say
> if I have added something to my tasklist or not so that it's clear to you
> what I believe is actionable (and what I am agreeing to change) and what I
> believe needs stronger input from you before I do anything. Apologies if it
> seems a little robotic, just trying new things O:-)
>
> On that note: not added to tasklist: do we need the LaTeX handler? Do we
> need any others? Please confirm O:-)

See above.

> On Fri, Feb 14, 2025 at 7:05 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> I started to eyeball old and new generated output side by side.
>>
>> New table of contents shows one level, old two.  No objection; the
>> navigation thingie on the left is more useful anyway.
>>
>
> Unintentional, but if you like it, it's fine by me. Nothing added to my
> tasklist.

Mention in a commit message.

>> The new generator elides unreferenced types.  Generally good, but two
>> observations:
>>
>> * QapiErrorClass is unreferenced, but its members are mentioned in
>>   Errors sections.  QapiErrorClass serves as better than nothing error
>>   code documentation, but it's gone in the new doc.  So this is a minor
>>   regression.  We can figure out what to do about it later.
>>
>
> Right. I debated making the members references to that class, but recalled
> that you disliked this class and figured you'd not like such a change, so I
> just left it alone. I do not have cross-references for individual members
> of objects at all yet anyway, so this is definitely more work regardless.
>
> We could always create a pragma of some sort (or just hardcode a list) of
> items that must be documented regardless of if they're referenced or not.
> Please let me know your preference and I will add a "ticket" on my personal
> tasklist for this project to handle that at /some point/. Nothing added to
> my tasklist just yet.

Suggest to add something like "compensate for the loss of QapiErrorClass
documentation in the QEMU QMP Reference Manual".

>> * Section "QMP errors" is empty in the new doc, because its entire
>>   contents is elided.  I guess we should elide the section as well, but
>>   it's fine to leave that for later.
>>
>
> Adding to tasklist to elide empty modules, but "for later".

ACK

>> Old doc shows a definition's since information like any other section.
>> New doc has it in the heading box.  Looks prettier and uses much less
>> space.  Not sure the heading box is the best place, but it'll do for
>> now, we can always move it around later.
>>
>
> Agree, it's a strict improvement - there may be further improvements, but
> that is always true anyway. When we tackle "autogenerated since
> information" we can tackle the since display issues more meticulously. Or
> maybe we'll need do sooner because of conflicting info in branches or
> whatever else. I dunno, I'll burn that bridge when I get to it. Nothing
> added to tasklist.

ACK

>> The new doc's headings use "Struct" or "Union" where the old one uses
>> just "Object".  Let's keep "Object", please.
>>
>
> I was afraid you'd ask for this. OK, I think it's an easy change. Can I
> keep the index page segmented by object type still, though?
>
> I do find knowing the *type* of object to be helpful as a developer,

Can you explain why and how struct vs. union matters to you as a
developer?

>                                                                      though
> I understand that from the point of view of a QMP user, they're all just
> objects, so your request makes sense.

I'd prefer a single index.

> Replace JSON object type headers with "Object" instead of QAPI data types
> added to tasklist.

ACK

>> In the new doc, some member references are no longer rendered as such,
>> e.g. @on-source-error and @on-target-error in BackupCommon's note.
>> Another small regression.
>>
>
> Ah, I actually knew this one. I didn't implement special formatting for
> these yet. I do not have cross-references for individual members, so
> there's nothing to transform these *into* yet. If you'd like special
> rendering for them (fixed width, no link?) that's easy to accomplish. I am
> not yet sure where I will do that conversion.

Suggest the render them the same as before.

Have a look at BackupCommon's "Note" box in the old docs: the member
names appear to use a fixed-width font.

Peeking at old qapidoc.py...  it seems to rewrite @foo to ``foo``.

> Reminder/Note that in my KVM Forum branch, I did actually replace all
> @references that pointed to something actually cross-referenceable with an
> actual sphinx cross-reference, leaving only @member references behind.
>
> Nothing added to tasklist just yet.
>
>
>>
>> Union branches are busted in the new generator's output.  I know they
>> used to work, so I'm not worried about it.
>>
>
> Fixed in new push, sorry! An embarrassing mistake that took me aeons to
> spot.
>
>
>>
>> The new doc shows the return type, the old doc doesn't.  Showing it is
>> definitely an improvement, but we need to adjust the doc text to avoid
>> silliness like "Returns: SnapshotInfo – SnapshotInfo".
>>
>
> My KVM Forum branch actually corrected the QAPI documentation to remove
> pointless returns. I didn't include that with this series yet, it was long
> enough. This issue will be addressed solely through source documentation
> edits, of which I believe I already have a comprehensive patch for.
>
> Added to my tasklist: "Submit source documentation patches to remove
> pointless return documentation"

ACK

> It was my intent to submit those patches *afterwards*, but we can always do
> it before if you'd like. Let me know. (I don't know offhand how easy they
> are to extricate from my KVM Forum branch. I reserve the right to change my
> mind on being flexible depending on the answer there :p)

No need to decide or extricate right now.  Tasklist is good enough for
me.

>> The new doc shows Arguments / Members, Returns, and Errors in two-column
>> format.  Looks nice.  But for some reason, the two columns don't align
>> horizontally for Errors like they do for the others.  Certainly not a
>> blocker of anything, but we should try to fix it at some point.
>>
>
> Known issue. The reason is because we do not mandate a source documentation
> format for errors - by convention, it is a list. There is (or was?) one
> occurrence where it wasn't a list and I wrote a patch to change that. I
> don't recall right now if we merged that or not. The misalignment is a
> result of nesting a list inside of a list.

Commit b32a6b62a82 (qapi: nail down convention that Errors sections are
lists)

> If we *mandate* the source format, I gain the ability to "peel off the
> nesting", which will fix the alignment.

"Mandate" means changing "should be formatted as an rST list" into "must
be", plus enforcement.  Works for me.

> Added to tasklist: "Address vertical misalignment in Errors formatting"

ACK, low priority.

> Not added: how? need more input from you, please.
>
>
>>
>> The new doc doesn't show non-definition conditionals, as mentioned in
>> the cover letter.  It shows definition conditionals twice.  Once should
>> suffice.
>>
>
> Known/intentional issue. I couldn't decide where I wanted it, so I put it
> in both places. If you have a strong opinion right now, please let me know
> what it is and I'll take care of it, it's easy - but it's code in the
> predecessor series and nothing to do with qapidoc, so please put it out of
> mind for now.
>
> If you don't have strong feelings, or you feel that the answer may depend
> on how we solve other glaring issues (non-definition conditionals), let's
> wait a little bit before making a decision.
>
> Added to tasklist: "Remove the duplication of definition conditionals";
> left unspecified is how or in what direction :)

ACK

I'll try to make up my mind :)

>> There's probably more, but this is it for now.
>>
>>
>
> Tasklist:
>
>  For the qapi-domain (prequel!) series:
>   - Remove the duplication of definition conditionals
>
> For this (qapidoc) series:
>   - Display all JSON object types as "Object" and not as their QAPI data
> type.
>
> For later:
>   - Elide empty modules
>   - Submit source documentation patches to remove pointless return
> documentation
>   - Address vertical misalignment in Errors formatting