Re: [PATCH 14/14] qapi/parser: convert single-section properties to getter properties

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> Remove dedicated pointers to single sections, instead preferring a
> functional getter property that returns the same thing dynamically.
>
> This is being done mainly for the benefit of being able to condense
> the _append() function, but I'm not convinced it's worth it. I'm not
> convinced it's not not worth it either, though.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/parser.py | 47 +++++++++++++++++++++++-------------------
>  1 file changed, 26 insertions(+), 21 deletions(-)

Hmm.

> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index 1d8f1d39ab1..d4b5b3e5ab6 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -773,12 +773,6 @@ def __init__(self, info: QAPISourceInfo, symbol: Optional[str] = None):
>          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
> -        # "Since" section
> -        self.since: Optional[QAPIDoc.Section] = None
> -
>      @property
>      def has_features(self) -> bool:
>          return bool(self._features)
> @@ -790,21 +784,10 @@ def _append(self, section: 'QAPIDoc.Section') -> None:
>          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:
> -                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
> +        if str(section.kind) in ('Intro', 'Returns', 'Errors', 'Since'):
> +            if self._sections[section.kind]:
> +                raise QAPISemError(section.info,
> +                                   "duplicated '%s' section" % section.kind)
>          self._sections[section.kind].append(section)
>          self._active_section = section
>  

Likable so far.

> @@ -815,6 +798,28 @@ def __iter__(self) -> Iterator['QAPIDoc.Section']:
>      def __len__(self) -> int:
>          return sum(len(x) for x in self._sections.values())
>  
> +    def _single_section_property(
> +        self,
> +        kind: 'QAPIDoc.Kind'
> +    ) -> Optional['QAPIDoc.Section']:
> +        try:
> +            return self._sections[kind][0]
> +        except IndexError:
> +            pass
> +        return None

Returns the first section of a kind.  Let's name it after what it does:
_first_section().

The try works, but

           return self._sections[kind][0] if self._sections[kind] else None

seems to work as well, and with less ceremony :)

> +
> +    @property
> +    def _returns(self) -> Optional['QAPIDoc.Section']:
> +        return self._single_section_property(QAPIDoc.Kind.RETURNS)
> +
> +    @property
> +    def _errors(self) -> Optional['QAPIDoc.Section']:
> +        return self._single_section_property(QAPIDoc.Kind.ERRORS)
> +
> +    @property
> +    def since(self) -> Optional['QAPIDoc.Section']:
> +        return self._single_section_property(QAPIDoc.Kind.SINCE)
> +
>      def end(self) -> None:
>          for section in self:
>              section.text = section.text.strip('\n')