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:
[...]