Re: [PATCH 11/14] qapi/parser: privatize "all_sections" field as "_sections"

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> In preparation for switching QAPIDoc to a more complex data structure
> for keeping track of sections, convert the "all_sections" property to
> a private property.

s/property/attribute/

> Rename it "_sections" to emphasize that there is not* some alternative
> property that is somehow not all sections. This is what you get and
> you're going to love it.
>
> Implement the len() and iter() features for QAPIDoc as a replacement
> for direct, public access to the all_sections property.
>
> (*OK, currently there are some, but spoilers, they're going away in
> the next commits.)
>
> Signed-off-by: John Snow <jsnow@redhat.com>

This took a bit of work for me to understand.

I figure "alternative that is somehow not all sections" refers to .args,
._features, ._returns, ._errors, and .since.

You replace the latter three by properties in PATCH 14.  The former two
stay.  "They're going away" is not entirely true.

Or am I confused?

Here'a another way to justify the renaming.  When I added .all_sections
in commit 3d035cd2cca, there was also .sections, which held only the
"sections other than .body, .args, .features".  You dropped .sections in
commit d678c5aff31.  No real need for "all_" since.

> ---
>  docs/sphinx/qapidoc.py         |  7 ++++---
>  scripts/qapi/parser.py         | 27 +++++++++++++++++----------
>  tests/qapi-schema/test-qapi.py |  2 +-
>  3 files changed, 22 insertions(+), 14 deletions(-)
>
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index ae57056a393..fd666be131f 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -377,7 +377,7 @@ def visit_sections(self, ent: QAPISchemaDefinition) -> None:
>                  )
>              )
>  
> -        sections = ent.doc.all_sections if ent.doc else []
> +        sections = list(ent.doc) if ent.doc else []

Why listify?  All you need is an iterator as far as I can tell.

>  
>          # Add sections in source order:
>          for section in sections:
> @@ -416,8 +416,9 @@ def visit_module(self, path: str) -> None:
>          self.ensure_blank_line()
>  
>      def visit_freeform(self, doc: QAPIDoc) -> None:
> -        assert len(doc.all_sections) == 1, doc.all_sections
> -        body = doc.all_sections[0]
> +        sections = list(doc)
> +        assert len(sections) == 1, sections
> +        body = sections[0]
>          self.add_lines(self.reformat_arobase(body.text), doc.info)
>          self.ensure_blank_line()

The code here checks "single section", then formats that section.

Works because a free-form doc comment is a single DETAILS section.

Aside: calling it DETAILS is a bit odd, but we'll live.

Aside: if we recognized TODO: there, it could be several, but we don't.
Maybe we should.

Your patch looks fine to me, but I wonder whether we could instead do

           for section in doc:
               assert section.kind == QAPIDoc.Kind.DETAILS
               self.add_lines(self.reformat_arobase(section.text), section.info)
               self.ensure_blank_line()

Separate cleanup patch, I guess.

>  
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index 0dca1acba4c..bdd0f1ec3ff 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -19,6 +19,7 @@
>      TYPE_CHECKING,
>      Any,
>      Dict,
> +    Iterator,
>      List,
>      Mapping,
>      Match,
> @@ -758,7 +759,7 @@ def __init__(self, info: QAPISourceInfo, symbol: Optional[str] = None):
>          # definition doc's symbol, None for free-form doc
>          self.symbol: Optional[str] = symbol
>          # the sections in textual order
> -        self.all_sections: List[QAPIDoc.Section] = []
> +        self._sections: List[QAPIDoc.Section] = []
>          # dicts mapping parameter/feature names to their description
>          self._args: Dict[str, QAPIDoc.ArgSection] = {}
>          self._features: Dict[str, QAPIDoc.ArgSection] = {}
> @@ -797,10 +798,16 @@ def _append(self, section: 'QAPIDoc.Section') -> None:
>                  raise QAPISemError(
>                      section.info, "duplicated '%s' section" % section.kind)
>              self.since = section
> -        self.all_sections.append(section)
> +        self._sections.append(section)
> +
> +    def __iter__(self) -> Iterator['QAPIDoc.Section']:
> +        return iter(self._sections)
> +
> +    def __len__(self) -> int:
> +        return len(self._sections)
>  
>      def end(self) -> None:
> -        for section in self.all_sections:
> +        for section in self:
>              section.text = section.text.strip('\n')
>              if (
>                      section.kind not in (
> @@ -813,9 +820,9 @@ def end(self) -> None:
>      def ensure_untagged_section(self, info: QAPISourceInfo) -> None:
>          kind = QAPIDoc.Kind.DETAILS
>  
> -        if self.all_sections and self.all_sections[-1].kind == kind:
> +        if self._sections and self._sections[-1].kind == kind:
>              # extend current section
> -            section = self.all_sections[-1]
> +            section = self._sections[-1]
>              if not section.text:
>                  # Section is empty so far; update info to start *here*.
>                  section.info = info
> @@ -856,7 +863,7 @@ def new_feature(self, info: QAPISourceInfo, name: str) -> None:
>          self._new_description(info, name, QAPIDoc.Kind.FEATURE, self._features)
>  
>      def append_line(self, line: str) -> None:
> -        self.all_sections[-1].append_line(line)
> +        self._sections[-1].append_line(line)
>  
>      def _insert_near_kind(
>          self,
> @@ -865,12 +872,12 @@ def _insert_near_kind(
>          after: bool = False,
>      ) -> bool:
>          """Insert or append a new doc section at a specific point."""
> -        for idx, sect in enumerate(reversed(self.all_sections)):
> +        for idx, sect in enumerate(reversed(list(self))):

Having to listify an iterator just to run it backwards is always sad.
Fortunately, this code is about to go away.

>              if sect.kind == kind:
> -                pos = len(self.all_sections) - idx - 1
> +                pos = len(self) - idx - 1
>                  if after:
>                      pos += 1
> -                self.all_sections.insert(pos, new_sect)
> +                self._sections.insert(pos, new_sect)
>                  return True
>          return False
>  
> @@ -915,7 +922,7 @@ def ensure_returns(self, info: QAPISourceInfo) -> None:
>          # TODO: Take advantage of the new strict section ordering and
>          # simplify this algorithm.
>  
> -        if any(s.kind == QAPIDoc.Kind.RETURNS for s in self.all_sections):
> +        if self._returns:
>              return

Instead of translating the loop, you replace it.  I suspect the loop was
always unnecessary.  Consider a separate commit.

>  
>          # Stub "Returns" section for undocumented returns value
> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
> index 5beb96e894f..d5b4ac3a682 100755
> --- a/tests/qapi-schema/test-qapi.py
> +++ b/tests/qapi-schema/test-qapi.py
> @@ -117,7 +117,7 @@ def test_frontend(fname):
>              print('doc symbol=%s' % doc.symbol)
>          else:
>              print('doc freeform')
> -        for section in doc.all_sections:
> +        for section in doc:
>              if isinstance(section, QAPIDoc.ArgSection):
>                  print('    %s=%s' % (section.kind, section.name))
>              else: