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")
+ 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:
--
2.44.0John 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.
© 2016 - 2024 Red Hat, Inc.