[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v5 08/20] scripts/qapi/parser.py: improve doc comment indent
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v5 08/20] scripts/qapi/parser.py: improve doc comment indent handling |
Date: |
Tue, 22 Sep 2020 09:27:10 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
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.