| Message ID | 20240514215740.940155-8-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi: new sphinx qapi domain pre-requisites | expand |
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. 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? 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? 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(). > > 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: [...]
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. 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. > > > > 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: > > [...] > >
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: >> >> [...] >> >>
On Thu, Jun 13, 2024 at 10:46 AM Markus Armbruster <armbru@redhat.com> wrote: > 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. > 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: > >> > >> [...] > >> > >> > >
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 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: @@ -676,7 +682,7 @@ def ensure_untagged_section(self, info: QAPISourceInfo) -> None: self.all_sections.append(section) def new_tagged_section(self, info: QAPISourceInfo, tag: str) -> None: - section = self.Section(info, tag) + section = self.Section(info, tag, "tagged") if tag == 'Returns': if self.returns: raise QAPISemError( @@ -696,20 +702,21 @@ def new_tagged_section(self, info: QAPISourceInfo, tag: str) -> None: self.all_sections.append(section) def _new_description(self, info: QAPISourceInfo, name: str, + kind: str, desc: Dict[str, ArgSection]) -> None: if not name: raise QAPISemError(info, "invalid parameter name") if name in desc: raise QAPISemError(info, "'%s' parameter name duplicated" % name) - section = self.ArgSection(info, '@' + name) + section = self.ArgSection(info, '@' + name, kind) self.all_sections.append(section) desc[name] = section def new_argument(self, info: QAPISourceInfo, name: str) -> None: - self._new_description(info, name, self.args) + self._new_description(info, name, 'member', self.args) def new_feature(self, info: QAPISourceInfo, name: str) -> None: - self._new_description(info, name, self.features) + self._new_description(info, name, 'feature', self.features) def append_line(self, line: str) -> None: self.all_sections[-1].append_line(line) @@ -722,7 +729,7 @@ def connect_member(self, member: 'QAPISchemaMember') -> None: "%s '%s' lacks documentation" % (member.role, member.name)) self.args[member.name] = QAPIDoc.ArgSection( - self.info, '@' + member.name) + self.info, '@' + member.name, 'member') self.args[member.name].connect(member) def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
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(-)