Re: [PATCH 10/14] qapi/parser: add _append() method

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> This method is intended to become the sole point of appending new
> sections to the section list, prohibiting outside callers from
> accessing the list of sections directly.
>
> A forthcoming commit will actually convert the "all_sections" field to
> be a private field to begin enforcing that no outside callers are
> manipulating the section list from outside of this class.

This suggests you merely add the method now.  Not true, you also put it
to use.

>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/parser.py | 50 ++++++++++++++++++++++++++----------------
>  1 file changed, 31 insertions(+), 19 deletions(-)
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index 5fc9c18c81d..0dca1acba4c 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -542,7 +542,7 @@ def get_doc(self) -> 'QAPIDoc':
>              if not symbol:
>                  raise QAPIParseError(self, "name required after '@'")
>              doc = QAPIDoc(info, symbol)
> -            doc.all_sections.append(QAPIDoc.Section(info, QAPIDoc.Kind.INTRO))
> +            doc.new_tagged_section(info, QAPIDoc.Kind.INTRO)
>              line = self.get_doc_indented(doc)
>              no_more_args = False
>              last_ordered_section = QAPIDoc.Kind.INTRO
> @@ -772,6 +772,33 @@ def __init__(self, info: QAPISourceInfo, symbol: Optional[str] = None):
>      def has_features(self) -> bool:
>          return bool(self._features)
>  
> +    def _append(self, section: 'QAPIDoc.Section') -> None:

Maybe name it _append_section()?  We already have append_line()...

> +        """
> +        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.

Yes: ._insert_near_kind().

> +        """

This comment will soon change to

           """
           Primary internal interface for adding a new section.

           All other methods/helpers/etc ultimately use this and only
           this interface for appending a new section.
           """

There's a bit of redundancy.  Suggest

           """
           Add a new section at the end.

           It is intended to ...
           """

now, and

           """
           Add a new section at the end.

           All other ...
           """

later.

> +        if section.kind == QAPIDoc.Kind.RETURNS:
> +            if self._returns:
> +                raise QAPISemError(
> +                    section.info, "duplicated '%s' section" % section.kind)
> +            self._returns = section
> +        elif section.kind == QAPIDoc.Kind.ERRORS:
> +            if self._errors:
> +                raise QAPISemError(
> +                    section.info, "duplicated '%s' section" % section.kind)
> +            self._errors = section
> +        elif section.kind == QAPIDoc.Kind.SINCE:
> +            if self.since:
> +                raise QAPISemError(
> +                    section.info, "duplicated '%s' section" % section.kind)
> +            self.since = section
> +        self.all_sections.append(section)
> +
>      def end(self) -> None:
>          for section in self.all_sections:
>              section.text = section.text.strip('\n')
> @@ -797,7 +824,7 @@ def ensure_untagged_section(self, info: QAPISourceInfo) -> None:
>  
>          # start new section
>          section = self.Section(info, kind)
> -        self.all_sections.append(section)
> +        self._append(section)
>  
>      def new_tagged_section(
>          self,
> @@ -805,22 +832,7 @@ def new_tagged_section(
>          kind: 'QAPIDoc.Kind',
>      ) -> None:
>          section = self.Section(info, kind)
> -        if kind == QAPIDoc.Kind.RETURNS:
> -            if self._returns:
> -                raise QAPISemError(
> -                    info, "duplicated '%s' section" % kind)
> -            self._returns = section
> -        elif kind == QAPIDoc.Kind.ERRORS:
> -            if self._errors:
> -                raise QAPISemError(
> -                    info, "duplicated '%s' section" % kind)
> -            self._errors = section
> -        elif kind == QAPIDoc.Kind.SINCE:
> -            if self.since:
> -                raise QAPISemError(
> -                    info, "duplicated '%s' section" % kind)
> -            self.since = section
> -        self.all_sections.append(section)
> +        self._append(section)
>  
>      def _new_description(
>          self,
> @@ -834,7 +846,7 @@ def _new_description(
>          if name in desc:
>              raise QAPISemError(info, "'%s' parameter name duplicated" % name)
>          section = self.ArgSection(info, kind, name)
> -        self.all_sections.append(section)
> +        self._append(section)
>          desc[name] = section
>  
>      def new_argument(self, info: QAPISourceInfo, name: str) -> None: