| Message ID | 20250205231208.1480762-1-jsnow@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | docs: add sphinx-domain rST generator to qapidoc | expand |
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. 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. * 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. 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. The new doc's headings use "Struct" or "Union" where the old one uses just "Object". Let's keep "Object", please. 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. Union branches are busted in the new generator's output. I know they used to work, so I'm not worried about it. 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". 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. The new doc doesn't show non-definition conditionals, as mentioned in the cover letter. It shows definition conditionals twice. Once should suffice. There's probably more, but this is it for now.
"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. 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. (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:-) 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. > > 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. > > * 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". > > 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. > > 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, though I understand that from the point of view of a QMP user, they're all just objects, so your request makes sense. Replace JSON object type headers with "Object" instead of QAPI data types added to tasklist. > > 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. 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" 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) > > 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. If we *mandate* the source format, I gain the ability to "peel off the nesting", which will fix the alignment. Added to tasklist: "Address vertical misalignment in Errors formatting" 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 :) > > 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
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
On Wed, Feb 19, 2025 at 8:22 AM Markus Armbruster <armbru@redhat.com> wrote: > 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. > 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 <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. > 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 > >
John Snow <jsnow@redhat.com> writes: > On Wed, Feb 19, 2025 at 8:22 AM Markus Armbruster <armbru@redhat.com> wrote: > >> 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. >> > > 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. Understand. Have you considered cutting the series in half before the inliner? First part emits "The members of ..." like the old doc generator. Second part replaces that with inlined material. We could totally release with just the first half! Inlining is great, but even without it, your work looks so much better and is so much more usable. >> > 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. >> > > 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 think we could even do it after "turn on". Yes, it's a small regression, but I believe the improvements are big enough to outweigh small regressions like this one. > 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." [...] >> >> 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. No, only structs and unions are "Object", alternates are "Alternate". For me, the separation between struct and union is an unfortunate remnant of somewhat winding development history. A union is has common members, one of them is the tag, and for each tag value, it may have variant members. A struct is a degenerate union: no variants. This is as old as the hills: Pascal records are just like this. QMP introspection doesn't show structs and unions, just objects, which may or may not have variants. The schema language syntax, however, is still rooted (stuck?) in a past when unions could not have common members other than the tag. [...] >> >> 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. Understood. [...]
On Fri, Feb 21, 2025 at 1:42 AM Markus Armbruster <armbru@redhat.com> wrote: > John Snow <jsnow@redhat.com> writes: > > > On Wed, Feb 19, 2025 at 8:22 AM Markus Armbruster <armbru@redhat.com> > wrote: > > > >> 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. > >> > > > > 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. > > Understand. > > Have you considered cutting the series in half before the inliner? > First part emits "The members of ..." like the old doc generator. > Second part replaces that with inlined material. > > We could totally release with just the first half! Inlining is great, > but even without it, your work looks so much better and is so much more > usable. > I may indeed just do that... though we still need to solve "where to put the ifcond data?" question. The documentation culling also must be held back in this case too, which I am fine with. Let me fork my work (again) and see how complicated an inlinerless version would be... maybe that's a great way to flush the queue. maybe. > > >> > 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. > >> > > > > Sure. I... just need to figure out which commit to mention it in. Added > to > > my list, anyway. > It turns out this happens in the "example" doc patch, it's just a setting in index.rst. I didn't even intend to commit that patch anyway. So this is a nothing-burger. > > > > > >> > >> >> 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 think we could even do it after "turn on". Yes, it's a small > regression, but I believe the improvements are big enough to outweigh > small regressions like this one. > OK. > > > 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." > > [...] > > >> >> 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. > > No, only structs and unions are "Object", alternates are "Alternate". > > For me, the separation between struct and union is an unfortunate > remnant of somewhat winding development history. > > A union is has common members, one of them is the tag, and for each tag > value, it may have variant members. > > A struct is a degenerate union: no variants. > > This is as old as the hills: Pascal records are just like this. > > QMP introspection doesn't show structs and unions, just objects, which > may or may not have variants. > > The schema language syntax, however, is still rooted (stuck?) in a past > when unions could not have common members other than the tag. > OK, I can combine these two easily then. I see why you feel it isn't worth knowing the difference for developers either under this view. > > [...] > > >> >> 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. > > Understood. > > [...] > >
based-on: https://patchew.org/QEMU/20241213011307.2942030-1-jsnow@redhat.com/ Hiya! This series is based on a rebased version of the above series. Apply the above patches to origin/master and then apply this series and you should be good to go. Or just snitch the patches from my GitLab branch: https://gitlab.com/jsnow/qemu/-/commits/sphinx-domain-blergh2 (... ignore the branch name. I ran into some problems with stacked git corrupting my branches ...) If you're just tuning in, this series adds a new sphinx documentation generator for QAPI as presented on at KVM Forum; the big win here is cross-references and indices for QMP commands and events. It depends on the qapi-domain plugin for sphinx which is posted in the pre-requisite series; it's not polished for review and should be considered POC-quality. I felt it was easier to review this series backwards, because the design of the rST generator informs the design of the domain plugin. (Which makes sense: the rST is generated first, and then it's parsed. Our review follows the flow of data through the generator.) Overview: Patches 1-24: Mostly the same as in v2; implements the very basics of the new qapidoc generator. "type" was changed to "kind" for the doc section metadata, and "untagged" changed to "plain". Some small phrasing tweaks here and there. Patches 25-26: Add auto-generated stub docs for undocumented members. Patches 27-29: Restrict the source QAPIDoc syntax slightly and differentiate "plain" sections as either intro or details. Necessary for the inliner. Patch 30: Add the "inliner", the component that squishes "inherited" arguments/members into a single reference for commands/events. Patches 31-32: Add auto-generated documentation for commands that return a value that is not documented. Patches 33-35: Document the "out-of-band" property on QMP commands. Patches 36-38: Add branch support to the inliner. Ish. See below. Patches 39-40: Cull unused definitions from the generated QMP docs; cull anything that has been inlined and no longer needs to be documented separately. Patches 41-42: Add intermediate representation rST document writing in DEBUG mode Things notably still not perfect: (ignoring aesthetics; we care only about the rST generator itself in this series.) - ifcond for anything other than root level entires is still ignored - branch inliner ignores all sections except members (ifcond, details, features) - intro/details separation enforces no plain paragraphs to appear in the "middle" of the documentation section; new markup may be desired if we want to add annotations to categories/regions instead of to specific members/features. If you want to give this a whirl yourself, build QEMU with documentation support enabled and look at docs/manual/qapi/index.html for a sample generation of the QMP manual using the new system. You probably need sphinx >= 4.0 for the time being to do so. John Snow (42): docs/qapidoc: support header-less freeform sections qapi/parser: adjust info location for doc body section docs/qapidoc: remove example section support qapi: expand tags to all doc sections qapi/schema: add __repr__ to QAPIDoc.Section docs/qapidoc: add transmogrifier stub docs/qapidoc: add transmogrifier class stub docs/qapidoc: add visit_module() method qapi/source: allow multi-line QAPISourceInfo advancing docs/qapidoc: add visit_freeform() method docs/qapidoc: add preamble() method docs/qapidoc: add visit_paragraph() method docs/qapidoc: add visit_errors() method docs/qapidoc: add format_type() method docs/qapidoc: add add_field() and generate_field() helper methods docs/qapidoc: add visit_feature() method docs/qapidoc: prepare to record entity being transmogrified docs/qapidoc: add visit_returns() method docs/qapidoc: add visit_member() method docs/qapidoc: add visit_sections() method docs/qapidoc: add visit_entity() docs/qapidoc: implement transmogrify() method docs: disambiguate cross-references docs/qapidoc: add transmogrifier test document docs/qapidoc: generate entries for undocumented members qapi/parser: add undocumented stub members to all_sections qapi: differentiate "intro" and "detail" sections qapi/parser: prohibit untagged sections between tagged sections qapi: Add "Details:" disambiguation marker docs/qapidoc: add minimalistic inliner docs/qapidoc: autogenerate undocumented return docs docs/qapidoc: Add generated returns documentation to inliner docs/qmp: add target to Out-of-band execution section docs/qapidoc: document the "out-of-band" pseudofeature docs/qapidoc: generate out-of-band pseudofeature sections qapi/parser: add "meta" kind to QAPIDoc.Kind qapi/schema: add __iter__ method to QAPISchemaVariants docs/qapi: add branch support to inliner qapi/schema: add doc_visible property to QAPISchemaDefinition docs/qapidoc: cull (most) un-named entities from docs qapi: resolve filenames in info structures docs/qapidoc: add intermediate output debugger docs/devel/codebase.rst | 6 +- docs/glossary.rst | 10 +- docs/index.rst | 1 + docs/interop/qmp-spec.rst | 2 + docs/qapi/index.rst | 53 +++ docs/sphinx/qapidoc.py | 716 ++++++++++++++++++++++++++++++-- qapi/machine.json | 2 + qapi/migration.json | 4 + qapi/net.json | 4 +- qapi/qom.json | 8 +- qapi/yank.json | 2 + scripts/qapi/introspect.py | 4 +- scripts/qapi/parser.py | 178 ++++++-- scripts/qapi/schema.py | 48 ++- scripts/qapi/source.py | 4 +- scripts/qapi/types.py | 4 +- scripts/qapi/visit.py | 4 +- tests/qapi-schema/doc-good.json | 4 +- tests/qapi-schema/doc-good.out | 12 +- tests/qapi-schema/doc-good.txt | 8 +- tests/qapi-schema/test-qapi.py | 4 +- 21 files changed, 975 insertions(+), 103 deletions(-) create mode 100644 docs/qapi/index.rst