On Thu, Jun 13, 2024 at 10:46 AM Markus Armbruster <[email protected]> wrote:
> John Snow <[email protected]> writes: > > > On Thu, May 16, 2024, 2:18 AM Markus Armbruster <[email protected]> > wrote: > > > >> John Snow <[email protected]> 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 <[email protected]> > >> > --- > >> > 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. > OK, knock yourself out :) > > > 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. > How do I name this when it comes to intro and outro sections? "intro-untagged", "outro-untagged"? I still think "paragraphs" makes more sense, unfortunately, ... I realize "untagged" is more consistent with what we have currently, but that's just not really where I'm trying to go with this stuff. Seeing as naming things is the hardest unsolved problem in computer science, got some concrete suggestions? --js > > > > >> > > >> > 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: > >> > >> [...] > >> > >> > >
