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

[Peter Maydell <peter.maydell@linaro.org>]

On Fri, 4 Sep 2020 at 10:03, Markus Armbruster <armbru@redhat.com> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > Make the handling of indentation in doc comments more sophisticated,

> >          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()

OK.

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

Fixed; new message produces reports like:
doc-bad-indent.json:6:1: unexpected de-indent (expected at least 4 spaces)

(I have not special-cased "1 spaces" -> "1 space"...)

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

Fixed.

> > +

> > @@ -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)

Fixed.

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

Yeah. This is because at this point 'name' is not actually just the
name "arg" but includes the leading '@' and trailing ':' so I got
confused between "we want the length of the name ("arg") plus 3"
and the expression you need to actually use. I got this right in the
comment in _append_args_line() but not in _append_features_line().
Will clarify (in both functions) to:

                # Line is "@arg: first line of description"; since 'name'
                # at this point is "@arg:" any following lines should be
                # indented by len(name) + 1. We pad out this first line
                # so it is handled the same way.

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

The assumption is that if you added extra whitespace characters that's
because you wanted to specify a line of rST which starts with leading
spaces. So the handling here is that if you say

@foo:   bar
      baz

it's because you want the rST to be

  bar
baz

If this turns out to be invalid rST then the rST parser will
find that out later on.

As it happens I'm not sure whether there is any useful rST
syntax which has leading spaces and where you'd want to be able
to start an argument docstring with it, but it means we're
consistent with our handling of free-form doc comments, where
writing

   Foo
   bar

and writing

Foo
bar

are different things. Also with the change you suggest later
to avoid special-casing the "Examples" section then literal
text becomes an example of where it makes a difference.

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

Hmm, so you'd rather we changed the documentation of that
command so that instead of

# Example: remove all latency histograms:
#
# -> { "execute": "block-latency-histogram-set",
#      "arguments": { "id": "drive0" } }
# <- { "return": {} }

it would be

# Example:
# remove all latency histograms:
#
# -> { "execute": "block-latency-histogram-set",
#      "arguments": { "id": "drive0" } }
# <- { "return": {} }

and remove the special-case for "Example" so that if you did
write

Example: something on the same line
         more stuff here

it would be treated as literal text

something on the same line
more stuff here

?

Seems reasonable. (I think I put this special case in only
because I was trying to avoid changes to the existing doc
comments if it was easy to accommodate them in the parser.)
That command does seem to be the only outlier, so I've added
a patch to v6 which will fix up its documentation comment
and dropped the special casing.

> > +            else:
> > +                # Line is "SectionName: some text", indent required
>
> Same situation as above, much terser comment.

Fixed to use the expanded comment from earlier.

> > +                indent = len(name) + 1
> > +                line = ' ' * indent + line
> > +            self._start_section(name[:-1], indent)
> >
> >          self._append_freeform(line)

> > @@ -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)

Fixed.

> >          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)
>

Yeah; this part of the patch used to be a "just update all the
callsites of QAPIDoc.ArgSection() to pass the extra argument"
hunk. It looks like your commit 8ec0e1a4e68781 removed this
callsite entirely as dead code, but I missed that in the rebase
and accidentally reintroduced the dead code. Fixed.

> 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' } }

The '@' at the front of the second line here is not relevant to
the mis-indentation and it's kind of confusing (as the correct
fix is "add a colon", not "reindent the line"), so I think I'd
rather have a test that's clearly looking at the indent:

# Multiline doc comments should have consistent indentation

##
# @foo:
# @a: line one
# line two is wrongly indented
##
{ 'command': 'foo', 'data': { 'a': 'int' } }

which expects the error:

doc-bad-indent.json:6:1: unexpected de-indent (expected at least 4 spaces)

thanks
-- PMM