Re: [PATCH 13/14] qapi/parser: add new _sections mapping

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> Modify the _sections field to be a mapping between section kinds and a
> list of sections instead of a simple, flat list.
>
> This makes it trivial to insert new auto-generated sections at the
> correct position in the collection instead of needing to iterate
> carefully over the list every time.
>
> As a result, a lot of helpers we had to help us manage these
> insertions are now unnecessary and can be deleted with extreme
> prejudice.
>
> NOTE: The remaining fishy use of a member insertion by the qapidoc
> generator still remains, though it uses a simpler code pathway
> now. The addition of the actual inliner mechanism will be the final
> piece to remove this hack.
>
> NOTE 2: Because "TODO:" sections are not strictly ordered in source,
> this new section collection mechanism just lumps them in at the
> end. For our generated documentation, this is functionally identical
> as we do not render those sections anyway, but there is a noticeable
> impact on our unit tests which *do* render TODO sections.

This takes a path between the two extremes:

1. Throw them away.  Makes them completely invisible to the tests.

2. Fully preserve order information somehow.

   Consider this doc comment from doc-good.json:

    ##
    # @cmd:
    #
    # @arg1:
    #     description starts on a new line,
    #     indented
    #
    # @arg2: description starts on the same line
    #     remainder indented differently
    #
    # Returns: @Object
    #
    # Errors: some
    #
    # Features:
    # @cmd-feat1: a feature
    # @cmd-feat2: another feature
    #
    # .. note:: @arg3 is undocumented
    #
    # TODO: frobnicate
    #
    # .. admonition:: Notes
    #
    #  - Lorem ipsum dolor sit amet
    #  - Ut enim ad minim veniam
    [...]
    # Since: 2.10
    ##

  Its sections are:

    kind                source order
    --------------------------------------------
    Intro               0
    Member=arg1         1
    Member=arg2         2
    Member=arg3         none (it's synthesized)
    Returns             3
    Errors              4
    Feature=cmd-feat1   5
    Feature=cmd-feat2   6
    Details             7
    TODO                8
    Details             9
    Since               10

  We could record the number in the right column.

  Or we could try to use existing .info.line instead.  Slight
  complication: synthesized section Member=arg3 has a funny value there.

Mind, I'm not making demands here, I'm throwing you ideas.

> NOTE 3: This patch is what makes it obvious as to why we need
> _append(), len(), and iter() implementations.
>
> Signed-off-by: John Snow <jsnow@redhat.com>

I'd like the commit message to point out that and how we don't lose
source order, except for TODO.

Before the patch, ._sections is a flat list in source order.

The patch turns it into a dict of lists.  The strict section ordering
established in prior patches ensures that walking the dict's lists in
order is in source order: we add sections in source order, all sections
of the same kind are together, we keep the dict elements (lists) in
source order by adding at the end, and dict order is insertion order
since Python 3.6.

Except for the TODOs, because these are not strictly ordered.

> ---
>  docs/sphinx/qapidoc.py         |  6 +--
>  scripts/qapi/parser.py         | 74 ++++++++--------------------------
>  tests/qapi-schema/doc-good.out |  4 +-
>  3 files changed, 20 insertions(+), 64 deletions(-)
>
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index fd666be131f..41acb560628 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -371,11 +371,7 @@ def visit_sections(self, ent: QAPISchemaDefinition) -> None:
>          # other users of the QAPISchema or QAPIDoc objects outside of
>          # the document generator. Fishy, but not harmful.
>          if ent.doc:
> -            ent.doc.append_member_stub(
> -                QAPIDoc.ArgSection(
> -                    ent.doc.info, QAPIDoc.Kind.MEMBER, "q_dummy"
> -                )
> -            )
> +            ent.doc.new_argument(ent.doc.info, "q_dummy")
>  
>          sections = list(ent.doc) if ent.doc else []
>  
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index 894b8979b9e..1d8f1d39ab1 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -758,8 +758,13 @@ def __init__(self, info: QAPISourceInfo, symbol: Optional[str] = None):
>          self.info = info
>          # definition doc's symbol, None for free-form doc
>          self.symbol: Optional[str] = symbol
> -        # the sections in textual order
> -        self._sections: List[QAPIDoc.Section] = []
> +
> +        # all sections arranged by kind. This is identical to source

All

> +        # order, with the exception of TODO sections and any generated
> +        # sections that are appended post-hoc.

post hoc (no dash), I think.

> +        self._sections: Dict[QAPIDoc.Kind, List[QAPIDoc.Section]] = {
> +            kind: [] for kind in QAPIDoc.Kind
> +        }
>  
>          # The most recently appended section, on which append_line operates
>          self._active_section: Optional[QAPIDoc.Section] = None
> @@ -767,6 +772,7 @@ def __init__(self, info: QAPISourceInfo, symbol: Optional[str] = None):
>          # dicts mapping parameter/feature names to their description
>          self._args: Dict[str, QAPIDoc.ArgSection] = {}
>          self._features: Dict[str, QAPIDoc.ArgSection] = {}
> +
>          # a command's "Returns" and "Errors" section
>          self._returns: Optional[QAPIDoc.Section] = None
>          self._errors: Optional[QAPIDoc.Section] = None

This looks out of place.

> @@ -781,11 +787,8 @@ def _append(self, section: 'QAPIDoc.Section') -> None:
>          """
>          Primary internal interface for adding a new section.
>  
> -        It is intended to become the sole access point for adding new
> -        sections to the QAPIDoc section list, but we are currently in
> -        a transitional phase where other methods access the section
> -        list directly to do insertions of generated sections at
> -        targeted points.
> +        All other methods/helpers/etc ultimately use this and only
> +        this interface for appending a new section.
>          """
>          if section.kind == QAPIDoc.Kind.RETURNS:
>              if self._returns:
> @@ -802,14 +805,15 @@ def _append(self, section: 'QAPIDoc.Section') -> None:
>                  raise QAPISemError(
>                      section.info, "duplicated '%s' section" % section.kind)
>              self.since = section
> -        self._sections.append(section)
> +        self._sections[section.kind].append(section)
>          self._active_section = section
>  
>      def __iter__(self) -> Iterator['QAPIDoc.Section']:

Worth a comment about source order?  There's already one next to
self._sections.

> -        return iter(self._sections)
> +        for values in self._sections.values():
> +            yield from values
>  
>      def __len__(self) -> int:
> -        return len(self._sections)
> +        return sum(len(x) for x in self._sections.values())

No longer used as far as I can tell.

>  
>      def end(self) -> None:
>          for section in self:
> @@ -870,32 +874,6 @@ def append_line(self, line: str) -> None:
>          assert self._active_section is not None
>          self._active_section.append_line(line)
>  
> -    def _insert_near_kind(
> -        self,
> -        kind: 'QAPIDoc.Kind',
> -        new_sect: 'QAPIDoc.Section',
> -        after: bool = False,
> -    ) -> bool:
> -        """Insert or append a new doc section at a specific point."""
> -        for idx, sect in enumerate(reversed(list(self))):
> -            if sect.kind == kind:
> -                pos = len(self) - idx - 1

This is where it's used before this patch.

> -                if after:
> -                    pos += 1
> -                self._sections.insert(pos, new_sect)
> -                return True
> -        return False
> -
> -    def append_member_stub(self, stub: 'QAPIDoc.Section') -> None:
> -        """
> -        Append a stub section after any Member sections.
> -        """
> -        if self._insert_near_kind(QAPIDoc.Kind.MEMBER, stub, True):
> -            return
> -
> -        # No MEMBER sections present. Insert after INTRO section.
> -        self._insert_near_kind(QAPIDoc.Kind.INTRO, stub, after=True)
> -
>      def connect_member(self, member: 'QAPISchemaMember') -> None:
>          if member.name not in self._args:
>              assert member.info
> @@ -908,7 +886,7 @@ def connect_member(self, member: 'QAPISchemaMember') -> None:
>              stub_section = QAPIDoc.ArgSection(
>                  self.info, QAPIDoc.Kind.MEMBER, member.name)
>              self._args[member.name] = stub_section
> -            self.append_member_stub(stub_section)
> +            self._append(stub_section)
>  
>          self._args[member.name].connect(member)
>  
> @@ -920,30 +898,12 @@ def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
>          self._features[feature.name].connect(feature)
>  
>      def ensure_returns(self, info: QAPISourceInfo) -> None:
> -        # This is more complicated than it ought to be.  The doc
> -        # parser should already know where a generated RETURNS section
> -        # should go.  It currently doesn't.
> -        #
> -        # TODO: Take advantage of the new strict section ordering and
> -        # simplify this algorithm.
> -
> -        if self._returns:
> +        if self._sections[QAPIDoc.Kind.RETURNS]:

Why not keep the more concise self._returns?

>              return
>  
>          # Stub "Returns" section for undocumented returns value
>          stub = QAPIDoc.Section(info, QAPIDoc.Kind.RETURNS)
> -
> -        if any(self._insert_near_kind(kind, stub, after) for kind, after in (
> -                # 1. If arguments, right after those.
> -                (QAPIDoc.Kind.MEMBER, True),
> -                # 2. Elif errors, right *before* those.
> -                (QAPIDoc.Kind.ERRORS, False),
> -                # 3. Elif features, right *before* those.
> -                (QAPIDoc.Kind.FEATURE, False),
> -                # 4. Else, right *after* the intro.
> -                (QAPIDoc.Kind.INTRO, True),
> -        )):
> -            return
> +        self._append(stub)
>  
>      def check_expr(self, expr: QAPIExpression) -> None:
>          if 'command' in expr:
> diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
> index 7a485e46037..4a0fae6fce2 100644
> --- a/tests/qapi-schema/doc-good.out
> +++ b/tests/qapi-schema/doc-good.out
> @@ -181,8 +181,6 @@ a feature
>  another feature
>      Details
>  .. note:: @arg3 is undocumented
> -    Todo
> -frobnicate
>      Details
>  .. admonition:: Notes
>  
> @@ -207,6 +205,8 @@ Examples::
>  
>  Note::
>      Ceci n'est pas une note
> +    Todo
> +frobnicate
>      Since
>  2.10
>  doc symbol=cmd-boxed