[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: |
Fri, 04 Sep 2020 11:03:36 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
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' } }