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:

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

In general, I'm wary of making the amount of whitespace within a line
significant, but in this case, the visual misalignment of bar and baz
should make accidents unlikely.

How does

  @foo:  bar
         baz
  @frob: gnu
         gnat

behave?

This is something people may actually write.

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

Valid points.

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

Sounds like a good trade.

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

Yes, that's better.