Re: [PATCH 07/20] qapi/parser: add semantic 'kind' parameter to QAPIDoc.Section

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Thu, May 16, 2024, 2:18 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@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 <jsnow@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:
>>
>> [...]
>>
>>