John Snow <js...@redhat.com> writes: > On Thu, May 16, 2024, 2:18 AM Markus Armbruster <arm...@redhat.com> wrote: > >> John Snow <js...@redhat.com> writes: >> >> > When iterating all_sections, this is helpful to be able to distinguish >> > "members" from "features"; the only other way to do so is to >> > cross-reference these sections against QAPIDoc.args or QAPIDoc.features, >> > but if the desired end goal for QAPIDoc is to remove everything except >> > all_sections, we need *something* accessible to distinguish them. >> > >> > To keep types simple, add this semantic parameter to the base Section >> > and not just ArgSection; we can use this to filter out paragraphs and >> > tagged sections, too. >> > >> > Signed-off-by: John Snow <js...@redhat.com> >> > --- >> > scripts/qapi/parser.py | 25 ++++++++++++++++--------- >> > 1 file changed, 16 insertions(+), 9 deletions(-) >> > >> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py >> > index 161768b8b96..cf4cbca1c1f 100644 >> > --- a/scripts/qapi/parser.py >> > +++ b/scripts/qapi/parser.py >> > @@ -613,21 +613,27 @@ class QAPIDoc: >> > >> > class Section: >> > # pylint: disable=too-few-public-methods >> > - def __init__(self, info: QAPISourceInfo, >> > - tag: Optional[str] = None): >> > + def __init__( >> > + self, >> > + info: QAPISourceInfo, >> > + tag: Optional[str] = None, >> > + kind: str = 'paragraph', >> > + ): >> > # section source info, i.e. where it begins >> > self.info = info >> > # section tag, if any ('Returns', '@name', ...) >> > self.tag = tag >> > # section text without tag >> > self.text = '' >> > + # section type - {paragraph, feature, member, tagged} >> > + self.kind = kind >> >> Hmm. .kind is almost redundant with .tag. >> > > Almost, yes. But the crucial bit is members/features as you notice. That's > the real necessity here that saves a lot of code when relying on *only* > all_sections. > > (If you want to remove the other fields leaving only all_sections behind, > this is strictly necessary.) > > >> Untagged section: .kind is 'paragraph', .tag is None >> >> Member description: .kind is 'member', .tag matches @NAME >> >> Feature description: .kind is 'feature', .tag matches @NAME > > >> Tagged section: .kind is 'tagged', .tag matches >> r'Returns|Errors|Since|Notes?|Examples?|TODO' >> >> .kind can directly be derived from .tag except for member and feature >> descriptions. And you want to tell these two apart in a straightforward >> manner in later patches, as you explain in your commit message. >> >> If .kind is 'member' or 'feature', then self must be an ArgSection. >> Worth a comment? An assertion? >> > > No real need. The classes don't differ much in practice so there's not much > benefit, and asserting it won't help the static typer out anyway because it > can't remember the inference from string->type anyway. > > If you wanted to be FANCY, we could use string literal typing on the field > and restrict valid values per-class, but that's so needless not even I'm > tempted by it. > > >> Some time back, I considered changing .tag for member and feature >> descriptions to suitable strings, like your 'member' and 'feature', and >> move the member / feature name into ArgSection. I didn't, because the >> benefit wasn't worth the churn at the time. Perhaps it's worth it now. >> Would it result in simpler code than your solution? >> > > Not considerably, I think. Would just be shuffling around which field names > I touch and where/when.
The way .tag works irks me. I might explore the change I proposed just to see whether I hate the result less. On top of your work, to not annoy you without need. > It might actually just add some lines where I have to assert isinstance to > do type narrowing in the generator. > >> Terminology nit: the section you call 'paragraph' isn't actually a >> paragraph: it could be several paragraphs. Best to call it 'untagged', >> as in .ensure_untagged_section(). >> > > Oh, I hate when you make a good point. I was avoiding the term because I'm > removing Notes and Examples, and we have plans to eliminate Since ... the > tagged sections are almost going away entirely, leaving just TODO, which we > ignore. > > Uhm, can I name it paragraphs? :) or open to other suggestions, incl. > untagged if that's still your preference. 'untagged' is more consistent with existing names and comments: .ensure_untagged_section(), "additional (non-argument) sections, possibly tagged", ... When all tags but 'TODO' are gone, the concept "tagged vs. untagged section" ceases to make sense, I guess. We can tweak the names and comments accordingly then. > >> > >> > def append_line(self, line: str) -> None: >> > self.text += line + '\n' >> > >> > class ArgSection(Section): >> > - def __init__(self, info: QAPISourceInfo, tag: str): >> > - super().__init__(info, tag) >> > + def __init__(self, info: QAPISourceInfo, tag: str, kind: str): >> > + super().__init__(info, tag, kind) >> > self.member: Optional['QAPISchemaMember'] = None >> > >> > def connect(self, member: 'QAPISchemaMember') -> None: >> >> [...] >> >>