Re: [PATCH v5 08/20] scripts/qapi/parser.py: improve doc comment indent handling

[Markus Armbruster <armbru@redhat.com>]

Peter Maydell <peter.maydell@linaro.org> writes:

> Make the handling of indentation in doc comments more sophisticated,
> so that when we see a section like:
>
> Notes: some text
>        some more text
>           indented line 3
>
> we save it for the doc-comment processing code as:
>
> some text
> some more text
>    indented line 3
>
> and when we see a section with the heading on its own line:
>
> Notes:
>
> some text
> some more text
>    indented text
>
> we also accept that and save it in the same form.
>
> The exception is that we always retain indentation as-is for Examples
> sections, because these are literal text.

Does docs/devel/qapi-code-gen.txt need an update?  Hmm, looks like you
leave it to [PATCH 15] docs/devel/qapi-code-gen.txt: Update to new rST
backend conventions.  Acceptable.  Mentioning it in the commit message
now may make sense.

> If we detect that the comment document text is not indented as much
> as we expect it to be, we throw a parse error.  (We don't complain
> about over-indented sections, because for rST this can be legitimate
> markup.)
>
> The golden reference for the doc comment text is updated to remove
> the two 'wrong' indents; these now form a test case that we correctly
> stripped leading whitespace from an indented multi-line argument
> definition.
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> v1->v2: Update doc-good.out as per final para.
> ---
>  scripts/qapi/parser.py         | 81 +++++++++++++++++++++++++++-------
>  tests/qapi-schema/doc-good.out |  4 +-
>  2 files changed, 67 insertions(+), 18 deletions(-)
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index 7fae4478d34..d9f11eadd96 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -308,18 +308,32 @@ class QAPIDoc:
>      """
>  
>      class Section:
> -        def __init__(self, name=None):
> +        def __init__(self, parser, name=None, indent=0):
> +            # parser, for error messages about indentation
> +            self._parser = parser
>              # optional section name (argument/member or section name)
>              self.name = name
>              # the list of lines for this section
>              self.text = ''
> +            # the expected indent level of the text of this section
> +            self._indent = indent
>  
>          def append(self, line):
> +            # Strip leading spaces corresponding to the expected indent level
> +            # Blank lines are always OK.
> +            if line:
> +                spacecount = len(line) - len(line.lstrip(" "))

Works, but I'd prefer

                   indent = re.match(r'\s*', line).end()

> +                if spacecount > self._indent:
> +                    spacecount = self._indent
> +                if spacecount < self._indent:
> +                    raise QAPIParseError(self._parser, "unexpected de-indent")

New error needs test coverage.  I append a possible test.

Reporting the expected indentation might be helpful.

> +                line = line[spacecount:]

If you use self._indent instead of spacecount here (which I find
clearer), you don't need to cap spacecount at self._indent above.

> +
>              self.text += line.rstrip() + '\n'
>  
>      class ArgSection(Section):
> -        def __init__(self, name):
> -            super().__init__(name)
> +        def __init__(self, parser, name, indent=0):
> +            super().__init__(parser, name, indent)
>              self.member = None
>  
>          def connect(self, member):
> @@ -333,7 +347,7 @@ class QAPIDoc:
>          self._parser = parser
>          self.info = info
>          self.symbol = None
> -        self.body = QAPIDoc.Section()
> +        self.body = QAPIDoc.Section(parser)
>          # dict mapping parameter name to ArgSection
>          self.args = OrderedDict()
>          self.features = OrderedDict()
> @@ -438,7 +452,18 @@ class QAPIDoc:
>  
>          if name.startswith('@') and name.endswith(':'):
>              line = line[len(name)+1:]
> -            self._start_args_section(name[1:-1])
> +            if not line or line.isspace():
> +                # Line was just the "@arg:" header; following lines
> +                # are not indented
> +                indent = 0
> +                line = ''
> +            else:
> +                # Line is "@arg: first line of description"; following
> +                # lines should be indented by len(name) + 1, and we
> +                # pad out this first line so it is handled the same way
> +                indent = len(name) + 1
> +                line = ' ' * indent + line
> +            self._start_args_section(name[1:-1], indent)
>          elif self._is_section_tag(name):
>              self._append_line = self._append_various_line
>              self._append_various_line(line)
> @@ -460,7 +485,17 @@ class QAPIDoc:
>  
>          if name.startswith('@') and name.endswith(':'):
>              line = line[len(name)+1:]
> -            self._start_features_section(name[1:-1])
> +            if not line or line.isspace():
> +                # Line is just the "@name:" header, no ident for following lines

pycodestyle complains:
scripts/qapi/parser.py:489:80: E501 line too long (80 > 79 characters)

> +                indent = 0
> +                line = ''
> +            else:
> +                # Line is "@arg: first line of description"; following
> +                # lines should be indented by len(name) + 3, and we
> +                # pad out this first line so it is handled the same way
> +                indent = len(name) + 1

Comment claims + 3, code uses + 1.

Does this do the right thing when @arg: is followed by multiple
whitespace characters?

> +                line = ' ' * indent + line
> +            self._start_features_section(name[1:-1], indent)
>          elif self._is_section_tag(name):
>              self._append_line = self._append_various_line
>              self._append_various_line(line)
> @@ -493,11 +528,23 @@ class QAPIDoc:
>                                   % (name, self.sections[0].name))
>          if self._is_section_tag(name):
>              line = line[len(name)+1:]
> -            self._start_section(name[:-1])
> +            if not line or line.isspace():
> +                # Line is just "SectionName:", no indent for following lines
> +                indent = 0
> +                line = ''
> +            elif name.startswith("Example"):
> +                # The "Examples" section is literal-text, so preserve
> +                # all the indentation as-is
> +                indent = 0

Section "Example" is an exception.  Needs to be documented.  Do we
really need the exception?  As far as I can see, it's only ever used in
documentation of block-latency-histogram-set.

> +            else:
> +                # Line is "SectionName: some text", indent required

Same situation as above, much terser comment.

> +                indent = len(name) + 1
> +                line = ' ' * indent + line
> +            self._start_section(name[:-1], indent)
>  
>          self._append_freeform(line)
>  
> -    def _start_symbol_section(self, symbols_dict, name):
> +    def _start_symbol_section(self, symbols_dict, name, indent):
>          # FIXME invalid names other than the empty string aren't flagged
>          if not name:
>              raise QAPIParseError(self._parser, "invalid parameter name")
> @@ -506,21 +553,21 @@ class QAPIDoc:
>                                   "'%s' parameter name duplicated" % name)
>          assert not self.sections
>          self._end_section()
> -        self._section = QAPIDoc.ArgSection(name)
> +        self._section = QAPIDoc.ArgSection(self._parser, name, indent)
>          symbols_dict[name] = self._section
>  
> -    def _start_args_section(self, name):
> -        self._start_symbol_section(self.args, name)
> +    def _start_args_section(self, name, indent):
> +        self._start_symbol_section(self.args, name, indent)
>  
> -    def _start_features_section(self, name):
> -        self._start_symbol_section(self.features, name)
> +    def _start_features_section(self, name, indent):
> +        self._start_symbol_section(self.features, name, indent)
>  
> -    def _start_section(self, name=None):
> +    def _start_section(self, name=None, indent=0):
>          if name in ('Returns', 'Since') and self.has_section(name):
>              raise QAPIParseError(self._parser,
>                                   "duplicated '%s' section" % name)
>          self._end_section()
> -        self._section = QAPIDoc.Section(name)
> +        self._section = QAPIDoc.Section(self._parser, name, indent)
>          self.sections.append(self._section)
>  
>      def _end_section(self):
> @@ -543,7 +590,7 @@ class QAPIDoc:
>      def connect_member(self, member):
>          if member.name not in self.args:
>              # Undocumented TODO outlaw
> -            self.args[member.name] = QAPIDoc.ArgSection(member.name)
> +            self.args[member.name] = QAPIDoc.ArgSection(self._parser, member.name)

pycodestyle complains:
scripts/qapi/parser.py:593:80: E501 line too long (82 > 79 characters)


>          self.args[member.name].connect(member)
>  
>      def connect_feature(self, feature):
> @@ -551,6 +598,8 @@ class QAPIDoc:
>              raise QAPISemError(feature.info,
>                                 "feature '%s' lacks documentation"
>                                 % feature.name)
> +            self.features[feature.name] = QAPIDoc.ArgSection(self._parser,
> +                                                             feature.name)

pylint points out:
scripts/qapi/parser.py:601:12: W0101: Unreachable code (unreachable)

>          self.features[feature.name].connect(feature)
>  
>      def check_expr(self, expr):
> diff --git a/tests/qapi-schema/doc-good.out b/tests/qapi-schema/doc-good.out
> index 0ef85d959ac..bbf77b08dc3 100644
> --- a/tests/qapi-schema/doc-good.out
> +++ b/tests/qapi-schema/doc-good.out
> @@ -158,7 +158,7 @@ doc symbol=Alternate
>  
>      arg=i
>  an integer
> -    @b is undocumented
> +@b is undocumented
>      arg=b
>  
>      feature=alt-feat
> @@ -173,7 +173,7 @@ doc symbol=cmd
>  the first argument
>      arg=arg2
>  the second
> -       argument
> +argument
>      arg=arg3
>  
>      feature=cmd-feat1


Suggested new test doc-bad-deintent.json, cribbed from your PATCH 06 of
doc-good.json:

##
# @Alternate:
# @i: an integer
# @b is undocumented
##
{ 'alternate': 'Alternate',
  'data': { 'i': 'int', 'b': 'bool' } }