Re: [PATCH v2 03/13] qapi/docs: make remaining subsection members "private"

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@redhat.com> writes:

> These fields are used to provide error checking and internal logistics
> and should not be used by a user of the library to directly access
> documentation sections, so make them private.
>
> The "since" field alone is left public, as the qapidoc generator does
> use this field to pull that section out of the regular flow of the
> document.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/parser.py | 54 +++++++++++++++++++++++-------------------
>  1 file changed, 29 insertions(+), 25 deletions(-)
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index b33edbba74f..da4756a7424 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -555,7 +555,7 @@ def get_doc(self) -> 'QAPIDoc':
>                      break
>                  # Non-blank line, first of a section
>                  if line == 'Features:':
> -                    if doc.features:
> +                    if doc.has_features:
>                          raise QAPIParseError(
>                              self, "duplicated 'Features:' line")
>                      self.accept(False)
> @@ -570,7 +570,7 @@ def get_doc(self) -> 'QAPIDoc':
>                          if text:
>                              doc.append_line(text)
>                          line = self.get_doc_indented(doc)
> -                    if not doc.features:
> +                    if not doc.has_features:
>                          raise QAPIParseError(
>                              self, 'feature descriptions expected')
>                      no_more_args = True

These two hunks along with the one adding .has_features() below make the
patch not purely mechanical.  Separate patch, please.

Have you considered using a local bool variable instead?

> @@ -733,14 +733,18 @@ def __init__(self, info: QAPISourceInfo, symbol: Optional[str] = None):
>              QAPIDoc.Section(info, QAPIDoc.Kind.PLAIN)
>          ]
>          # dicts mapping parameter/feature names to their description
> -        self.args: Dict[str, QAPIDoc.ArgSection] = {}
> -        self.features: Dict[str, QAPIDoc.ArgSection] = {}
> +        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
> +        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)
> +
>      def end(self) -> None:
>          for section in self.all_sections:
>              section.text = section.text.strip('\n')
> @@ -771,15 +775,15 @@ def new_tagged_section(
>      ) -> None:
>          section = self.Section(info, kind)
>          if kind == QAPIDoc.Kind.RETURNS:
> -            if self.returns:
> +            if self._returns:
>                  raise QAPISemError(
>                      info, "duplicated '%s' section" % kind)
> -            self.returns = section
> +            self._returns = section
>          elif kind == QAPIDoc.Kind.ERRORS:
> -            if self.errors:
> +            if self._errors:
>                  raise QAPISemError(
>                      info, "duplicated '%s' section" % kind)
> -            self.errors = section
> +            self._errors = section
>          elif kind == QAPIDoc.Kind.SINCE:
>              if self.since:
>                  raise QAPISemError(
> @@ -803,16 +807,16 @@ def _new_description(
>          desc[name] = section
>  
>      def new_argument(self, info: QAPISourceInfo, name: str) -> None:
> -        self._new_description(info, name, QAPIDoc.Kind.MEMBER, self.args)
> +        self._new_description(info, name, QAPIDoc.Kind.MEMBER, self._args)
>  
>      def new_feature(self, info: QAPISourceInfo, name: str) -> None:
> -        self._new_description(info, name, QAPIDoc.Kind.FEATURE, self.features)
> +        self._new_description(info, name, QAPIDoc.Kind.FEATURE, self._features)
>  
>      def append_line(self, line: str) -> None:
>          self.all_sections[-1].append_line(line)
>  
>      def connect_member(self, member: 'QAPISchemaMember') -> None:
> -        if member.name not in self.args:
> +        if member.name not in self._args:
>              assert member.info
>              if self.symbol not in member.info.pragma.documentation_exceptions:
>                  raise QAPISemError(member.info,
> @@ -823,7 +827,7 @@ def connect_member(self, member: 'QAPISchemaMember') -> None:
>  
>              section = QAPIDoc.ArgSection(
>                  self.info, QAPIDoc.Kind.MEMBER, member.name)
> -            self.args[member.name] = section
> +            self._args[member.name] = section
>  
>              # Determine where to insert stub doc - it should go at the
>              # end of the members section(s), if any. Note that index 0
> @@ -835,14 +839,14 @@ def connect_member(self, member: 'QAPISchemaMember') -> None:
>                      index += 1
>              self.all_sections.insert(index, section)
>  
> -        self.args[member.name].connect(member)
> +        self._args[member.name].connect(member)
>  
>      def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
> -        if feature.name not in self.features:
> +        if feature.name not in self._features:
>              raise QAPISemError(feature.info,
>                                 "feature '%s' lacks documentation"
>                                 % feature.name)
> -        self.features[feature.name].connect(feature)
> +        self._features[feature.name].connect(feature)
>  
>      def ensure_returns(self, info: QAPISourceInfo) -> None:
>  
> @@ -883,18 +887,18 @@ def _insert_near_kind(
>  
>      def check_expr(self, expr: QAPIExpression) -> None:
>          if 'command' in expr:
> -            if self.returns and 'returns' not in expr:
> +            if self._returns and 'returns' not in expr:
>                  raise QAPISemError(
> -                    self.returns.info,
> +                    self._returns.info,
>                      "'Returns' section, but command doesn't return anything")
>          else:
> -            if self.returns:
> +            if self._returns:
>                  raise QAPISemError(
> -                    self.returns.info,
> +                    self._returns.info,
>                      "'Returns' section is only valid for commands")
> -            if self.errors:
> +            if self._errors:
>                  raise QAPISemError(
> -                    self.errors.info,
> +                    self._errors.info,
>                      "'Errors' section is only valid for commands")
>  
>      def check(self) -> None:
> @@ -914,5 +918,5 @@ def check_args_section(
>                          "do" if len(bogus) > 1 else "does"
>                      ))
>  
> -        check_args_section(self.args, 'member')
> -        check_args_section(self.features, 'feature')
> +        check_args_section(self._args, 'member')
> +        check_args_section(self._features, 'feature')