| Message ID | 20240514215740.940155-10-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi: new sphinx qapi domain pre-requisites | expand |
John Snow <jsnow@redhat.com> writes: > This helps simplify the doc generator if it doesn't have to check for > undocumented members. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > scripts/qapi/parser.py | 20 ++++++++++++++++++-- > 1 file changed, 18 insertions(+), 2 deletions(-) > > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py > index b1794f71e12..3cd8e7ee295 100644 > --- a/scripts/qapi/parser.py > +++ b/scripts/qapi/parser.py > @@ -740,8 +740,24 @@ def connect_member(self, member: 'QAPISchemaMember') -> None: > raise QAPISemError(member.info, > "%s '%s' lacks documentation" > % (member.role, member.name)) > - self.args[member.name] = QAPIDoc.ArgSection( > - self.info, '@' + member.name, 'member') > + > + # Insert stub documentation section for missing member docs. > + section = QAPIDoc.ArgSection( > + self.info, f"@{member.name}", "member") Although I like f-strings in general, I'd pefer to stick to '@' + member.name here, because it's simpler. Also, let's not change 'member' to "member". Existing practice: single quotes for string literals unless double quotes avoid escapes. Except English prose (like error messages) is always in double quotes. > + self.args[member.name] = section > + > + # Determine where to insert stub doc. If we have some member documentation, the member doc stubs clearly must go there. Inserting them at the end makes sense. Else we want to put them where the parser would accept real member documentation. "The parser" is .get_doc(). This is what it accepts (I'm prepared to explain this in detail if necessary): One untagged section Member documentation, if any Zero ore more tagged or untagged sections Feature documentation, if any Zero or more tagged or untagged sections If we there is no member documentation, this is One untagged section Zero ore more tagged or untagged sections Feature documentation, if any Zero or more tagged or untagged sections Note that we cannot have two adjacent untagged sections (we only create one if the current section isn't untagged; if it is, we extend it instead). Thus, the second section must be tagged or feature documentation. Therefore, the member doc stubs must go right after the first section. This is also where qapidoc.py inserts member documentation. > + index = 0 > + for i, sect in enumerate(self.all_sections): > + # insert after these: > + if sect.kind in ('intro-paragraph', 'member'): > + index = i + 1 > + # but before these: > + elif sect.kind in ('tagged', 'feature', 'outro-paragraph'): > + index = i > + break Can you describe what this does in English? As a specification; simply paraphrasing the code is cheating. I tried, and gave up. Above, I derived what I believe we need to do. It's simple enough: if we have member documentation, it starts right after the first (untagged) section, and the stub goes to the end of the member documentation. Else, the stub goes right after the first section. Code: index = 1; while self.all_sections[index].kind == 'member': index += 1 Of course future patches I haven't seen might change the invariants in ways that break my simple code. We'll see. > + self.all_sections.insert(index, section) > + > self.args[member.name].connect(member) > > def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
On Fri, Jun 14, 2024 at 4:53 AM Markus Armbruster <armbru@redhat.com> wrote: > John Snow <jsnow@redhat.com> writes: > > > This helps simplify the doc generator if it doesn't have to check for > > undocumented members. > > > > Signed-off-by: John Snow <jsnow@redhat.com> > > --- > > scripts/qapi/parser.py | 20 ++++++++++++++++++-- > > 1 file changed, 18 insertions(+), 2 deletions(-) > > > > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py > > index b1794f71e12..3cd8e7ee295 100644 > > --- a/scripts/qapi/parser.py > > +++ b/scripts/qapi/parser.py > > @@ -740,8 +740,24 @@ def connect_member(self, member: > 'QAPISchemaMember') -> None: > > raise QAPISemError(member.info, > > "%s '%s' lacks documentation" > > % (member.role, member.name)) > > - self.args[member.name] = QAPIDoc.ArgSection( > > - self.info, '@' + member.name, 'member') > > + > > + # Insert stub documentation section for missing member docs. > > + section = QAPIDoc.ArgSection( > > + self.info, f"@{member.name}", "member") > > Although I like f-strings in general, I'd pefer to stick to '@' + > member.name here, because it's simpler. > Tomayto, Tomahto. (OK.) > > Also, let's not change 'member' to "member". Existing practice: single > quotes for string literals unless double quotes avoid escapes. Except > English prose (like error messages) is always in double quotes. > OK. I realize I'm not consistent in this patch either, but I'll explain that my using double quotes here is a black-ism that is sneaking in the more I use it to auto-format my patches :) Maybe time for a flag day when I move scripts/qapi to python/qemu/qapi ... (Sorry, this type of stuff is ... invisible to me, and I really do rely on the linters to make sure I don't do this kind of thing.) > > > + self.args[member.name] = section > > + > > + # Determine where to insert stub doc. > > If we have some member documentation, the member doc stubs clearly must > go there. Inserting them at the end makes sense. > > Else we want to put them where the parser would accept real member > documentation. > > "The parser" is .get_doc(). This is what it accepts (I'm prepared to > explain this in detail if necessary): > > One untagged section > > Member documentation, if any > > Zero ore more tagged or untagged sections > > Feature documentation, if any > > Zero or more tagged or untagged sections > > If we there is no member documentation, this is > > One untagged section > > Zero ore more tagged or untagged sections > > Feature documentation, if any > > Zero or more tagged or untagged sections > > Note that we cannot have two adjacent untagged sections (we only create > one if the current section isn't untagged; if it is, we extend it > instead). Thus, the second section must be tagged or feature > documentation. > > Therefore, the member doc stubs must go right after the first section. > > This is also where qapidoc.py inserts member documentation. > > > + index = 0 > > + for i, sect in enumerate(self.all_sections): > > + # insert after these: > > + if sect.kind in ('intro-paragraph', 'member'): > > + index = i + 1 > > + # but before these: > > + elif sect.kind in ('tagged', 'feature', > 'outro-paragraph'): > > + index = i > > + break > > Can you describe what this does in English? As a specification; simply > paraphrasing the code is cheating. I tried, and gave up. > It inserts after any intro-paragraph or member section it finds, but before any tagged, feature, or outro-paragraph it finds. The loop breaks on the very first instance of tagged/feature/outro, exiting immediately and leaving the insertion index set to the first occurrence of such a section, so that the insertion will place the member documentation prior to that section. The loop doesn't break when it finds intro-paragraph or members, so it'll continue to tick upwards until it reaches the end of the list or it finds something disqualifying. > > Above, I derived what I believe we need to do. It's simple enough: if > we have member documentation, it starts right after the first (untagged) > section, and the stub goes to the end of the member documentation. > Else, the stub goes right after the first section. > > Code: > > index = 1; > while self.all_sections[index].kind == 'member': > index += 1 > Wellp, yeah. That's certainly less code :) I tossed in your algorithm alongside mine and asserted they were always equal, and they are, so... yup. I think the only possible concern here is if there is precisely one and only one section and 1 is beyond EOL, but that's easy to fix. It apparently doesn't happen in practice, but I can't presently imagine why it *couldn't* happen. I'll just write a comment explaining the assumptions that make your algo work (intro section always guaranteed even if empty; intro sections always collapse into one section, members must start at i:=1 if they exist at all, members must be contiguous.) > > Of course future patches I haven't seen might change the invariants in > ways that break my simple code. We'll see. > > > + self.all_sections.insert(index, section) > > + > > self.args[member.name].connect(member) > > > > def connect_feature(self, feature: 'QAPISchemaFeature') -> None: > > Now, for a critique of my own patch: this patch makes it difficult to audit all of the cases where intro vs outro paragraphs sections may be ambiguous because we automatically add members sections, so the warning yap I add later on catches less cases. It's possible we may want to add a warning yap about paragraph ambiguity directly to the parser, OR just decide we don't really care and we just *assume* and that it's fine. We can discuss this pointedly on a call next time, and I'll come prepared with examples and line numbers.... Or, if you'd prefer, you can get a written report so you can take your time reading in silence. --js
John Snow <jsnow@redhat.com> writes: > On Fri, Jun 14, 2024 at 4:53 AM Markus Armbruster <armbru@redhat.com> wrote: > >> John Snow <jsnow@redhat.com> writes: >> >> > This helps simplify the doc generator if it doesn't have to check for >> > undocumented members. >> > >> > Signed-off-by: John Snow <jsnow@redhat.com> >> > --- >> > scripts/qapi/parser.py | 20 ++++++++++++++++++-- >> > 1 file changed, 18 insertions(+), 2 deletions(-) >> > >> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py >> > index b1794f71e12..3cd8e7ee295 100644 >> > --- a/scripts/qapi/parser.py >> > +++ b/scripts/qapi/parser.py >> > @@ -740,8 +740,24 @@ def connect_member(self, member: 'QAPISchemaMember') -> None: >> > raise QAPISemError(member.info, >> > "%s '%s' lacks documentation" >> > % (member.role, member.name)) >> > - self.args[member.name] = QAPIDoc.ArgSection( >> > - self.info, '@' + member.name, 'member') >> > + >> > + # Insert stub documentation section for missing member docs. >> > + section = QAPIDoc.ArgSection( >> > + self.info, f"@{member.name}", "member") >> >> Although I like f-strings in general, I'd pefer to stick to '@' + >> member.name here, because it's simpler. > > Tomayto, Tomahto. (OK.) Apropos healthy vegetables: at some time, we might want to mass-convert to f-strings where they are easier to read. >> Also, let's not change 'member' to "member". Existing practice: single >> quotes for string literals unless double quotes avoid escapes. Except >> English prose (like error messages) is always in double quotes. >> > > OK. I realize I'm not consistent in this patch either, but I'll explain > that my using double quotes here is a black-ism that is sneaking in the > more I use it to auto-format my patches :) > > Maybe time for a flag day when I move scripts/qapi to python/qemu/qapi ... > > (Sorry, this type of stuff is ... invisible to me, and I really do rely on > the linters to make sure I don't do this kind of thing.) > > >> >> > + self.args[member.name] = section >> > + >> > + # Determine where to insert stub doc. >> >> If we have some member documentation, the member doc stubs clearly must >> go there. Inserting them at the end makes sense. >> >> Else we want to put them where the parser would accept real member >> documentation. >> >> "The parser" is .get_doc(). This is what it accepts (I'm prepared to >> explain this in detail if necessary): >> >> One untagged section >> >> Member documentation, if any >> >> Zero ore more tagged or untagged sections >> >> Feature documentation, if any >> >> Zero or more tagged or untagged sections >> >> If we there is no member documentation, this is >> >> One untagged section >> >> Zero ore more tagged or untagged sections >> >> Feature documentation, if any >> >> Zero or more tagged or untagged sections >> >> Note that we cannot have two adjacent untagged sections (we only create >> one if the current section isn't untagged; if it is, we extend it >> instead). Thus, the second section must be tagged or feature >> documentation. >> >> Therefore, the member doc stubs must go right after the first section. >> >> This is also where qapidoc.py inserts member documentation. >> >> > + index = 0 >> > + for i, sect in enumerate(self.all_sections): >> > + # insert after these: >> > + if sect.kind in ('intro-paragraph', 'member'): >> > + index = i + 1 >> > + # but before these: >> > + elif sect.kind in ('tagged', 'feature', 'outro-paragraph'): >> > + index = i >> > + break >> >> Can you describe what this does in English? As a specification; simply >> paraphrasing the code is cheating. I tried, and gave up. >> > > It inserts after any intro-paragraph or member section it finds, but before > any tagged, feature, or outro-paragraph it finds. > > The loop breaks on the very first instance of tagged/feature/outro, exiting > immediately and leaving the insertion index set to the first occurrence of > such a section, so that the insertion will place the member documentation > prior to that section. > > The loop doesn't break when it finds intro-paragraph or members, so it'll > continue to tick upwards until it reaches the end of the list or it finds > something disqualifying. > > >> >> Above, I derived what I believe we need to do. It's simple enough: if >> we have member documentation, it starts right after the first (untagged) >> section, and the stub goes to the end of the member documentation. >> Else, the stub goes right after the first section. >> >> Code: >> >> index = 1; >> while self.all_sections[index].kind == 'member': >> index += 1 >> > > Wellp, yeah. That's certainly less code :) > > I tossed in your algorithm alongside mine and asserted they were always > equal, and they are, so... yup. I think the only possible concern here is > if there is precisely one and only one section and 1 is beyond EOL, but > that's easy to fix. It apparently doesn't happen in practice, but I can't > presently imagine why it *couldn't* happen. > > I'll just write a comment explaining the assumptions that make your algo > work (intro section always guaranteed even if empty; intro sections always > collapse into one section, members must start at i:=1 if they exist at all, > members must be contiguous.) You could assert the first section exists and is untagged. And maybe assert if we have members, the first is at index 1. >> Of course future patches I haven't seen might change the invariants in >> ways that break my simple code. We'll see. >> >> > + self.all_sections.insert(index, section) >> > + >> > self.args[member.name].connect(member) >> > >> > def connect_feature(self, feature: 'QAPISchemaFeature') -> None: >> >> > Now, for a critique of my own patch: this patch makes it difficult to audit > all of the cases where intro vs outro paragraphs sections may be ambiguous > because we automatically add members sections, so the warning yap I add > later on catches less cases. > > It's possible we may want to add a warning yap about paragraph ambiguity > directly to the parser, OR just decide we don't really care and we just > *assume* and that it's fine. > > We can discuss this pointedly on a call next time, and I'll come prepared > with examples and line numbers.... Or, if you'd prefer, you can get a > written report so you can take your time reading in silence. Let's try whatever feels easier for you first.
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py index b1794f71e12..3cd8e7ee295 100644 --- a/scripts/qapi/parser.py +++ b/scripts/qapi/parser.py @@ -740,8 +740,24 @@ def connect_member(self, member: 'QAPISchemaMember') -> None: raise QAPISemError(member.info, "%s '%s' lacks documentation" % (member.role, member.name)) - self.args[member.name] = QAPIDoc.ArgSection( - self.info, '@' + member.name, 'member') + + # Insert stub documentation section for missing member docs. + section = QAPIDoc.ArgSection( + self.info, f"@{member.name}", "member") + self.args[member.name] = section + + # Determine where to insert stub doc. + index = 0 + for i, sect in enumerate(self.all_sections): + # insert after these: + if sect.kind in ('intro-paragraph', 'member'): + index = i + 1 + # but before these: + elif sect.kind in ('tagged', 'feature', 'outro-paragraph'): + index = i + break + self.all_sections.insert(index, section) + self.args[member.name].connect(member) def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
This helps simplify the doc generator if it doesn't have to check for undocumented members. Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/parser.py | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)