On Wed, Feb 19, 2025 at 8:22 AM Markus Armbruster <arm...@redhat.com> wrote:
> John Snow <js...@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. > I've got html and text working, text wasn't hard. I will give it a good college try on the LaTeX and man formats. Might be easy. The issue here is the custom node I introduced for the collapsible details sections which has no default handler in the generators. I'll have to learn more about that part of the API, I haven't interfaced with it much yet. > > > On Fri, Feb 14, 2025 at 7:05 AM Markus Armbruster <arm...@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. > Sure. I... just need to figure out which commit to mention it in. Added to my list, anyway. > > >> 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". > Got it. Possibly a "for later" task but not much later. It can always come after this first series, but before we "turn on" the new generator, if that makes sense. Just so we reach a quiescent point and flush the staggeringly large queue. I guess what I mean is: "Let's make sure what I've got here so far is good first, and then I'll start adding stuff." > > >> * 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? > I suppose it's just internal details that I like to know, but tend to find the HTML reference easier to work with than grepping through the qapi files. I'm gonna change it for you anyway because I agree it's not consistent with the philosophy of "end user QMP reference". Just feels like a tiny shame somehow. > > > > 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. > So ... structs, unions, alternates all condensed down to "Object", is that right? We get to keep command/enum/event separate, I assume. > > > 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``. > OK. > > > 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 :) > I should also point out, this is an issue in the domain and not the generator; the generated rst document doesn't have this duplication. So it's kind of a no-op while we look and consider this specific series, but it's still on my list when we go to look at the predecessor series. > > >> 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 > >
