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