John Snow <jsnow@redhat.com> writes:
> Leading and trailing whitespace are now discarded, addressing the FIXME
> comment. A new error is raised to detect this accidental case.
>
> Parsing for args sections is left alone here; the 'name' variable is
> moved into the only block where it is used.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
>
> ---
>
> Tangentially related to delinting in that removing 'FIXME' comments is a
> goal for pylint. My goal is to allow 'TODO' to be checked in, but
> 'FIXME' should be fixed prior to inclusion.
>
> Arbitrary, but that's life for you.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qapi/parser.py | 13 ++++++++-----
> tests/qapi-schema/doc-whitespace-leading-symbol.err | 1 +
> .../qapi-schema/doc-whitespace-leading-symbol.json | 6 ++++++
> tests/qapi-schema/doc-whitespace-leading-symbol.out | 0
> .../qapi-schema/doc-whitespace-trailing-symbol.err | 1 +
> .../qapi-schema/doc-whitespace-trailing-symbol.json | 6 ++++++
> .../qapi-schema/doc-whitespace-trailing-symbol.out | 0
> tests/qapi-schema/meson.build | 2 ++
> 8 files changed, 24 insertions(+), 5 deletions(-)
> create mode 100644 tests/qapi-schema/doc-whitespace-leading-symbol.err
> create mode 100644 tests/qapi-schema/doc-whitespace-leading-symbol.json
> create mode 100644 tests/qapi-schema/doc-whitespace-leading-symbol.out
> create mode 100644 tests/qapi-schema/doc-whitespace-trailing-symbol.err
> create mode 100644 tests/qapi-schema/doc-whitespace-trailing-symbol.json
> create mode 100644 tests/qapi-schema/doc-whitespace-trailing-symbol.out
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index bfd2dbfd9a2..2f93a752f66 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -549,18 +549,21 @@ def _append_body_line(self, line):
>
> Else, append the line to the current section.
> """
> - name = line.split(' ', 1)[0]
> - # FIXME not nice: things like '# @foo:' and '# @foo: ' aren't
> - # recognized, and get silently treated as ordinary text
> - if not self.symbol and not self.body.text and line.startswith('@'):
> - if not line.endswith(':'):
> + stripped = line.strip()
> +
> + if not self.symbol and not self.body.text and stripped.startswith('@'):
> + if not stripped.endswith(':'):
> raise QAPIParseError(self._parser, "line should end with ':'")
> + if not stripped == line:
> + raise QAPIParseError(
> + self._parser, "extra whitespace around symbol declaration")
This rejects both leading and trailing whitespace. Rejecting leading
whitespace is good. Rejecting trailing whitespace feels a bit pedantic,
and it might not extend to the related case I'll point out below.
err'd on the conservative side. Wasn't sure how permissive we really wanted to be.
Have you considered a regexp instead? Say
match = re.match(r'(\s*)@([^:]*)(:?)(\s*)(.*)$', line)
Then match.group(n) is
n=1 leading whitespace, if any
n=2 symbol
n=3 trailing colon, if any
n=4 trailing whitespace, if any
n=5 trailing text, if any
Omit the subgroups you don't need.
Sensible, for a more comprehensive refactoring.
> self.symbol = line[1:-1]
> # FIXME invalid names other than the empty string aren't flagged
> if not self.symbol:
> raise QAPIParseError(self._parser, "invalid name")
> elif self.symbol:
> # This is a definition documentation block
> + name = line.split(' ', 1)[0]
> if name.startswith('@') and name.endswith(':'):
> self._append_line = self._append_args_line
> self._append_args_line(line)
Same issue here, and in _append_args_line(). To reproduce, I hacked up
doc-good.json like so
diff --git a/tests/qapi-schema/doc-good.json b/tests/qapi-schema/doc-good.json
index 86dc25d2bd..977fcbad48 100644
--- a/tests/qapi-schema/doc-good.json
+++ b/tests/qapi-schema/doc-good.json
@@ -133,7 +133,7 @@
##
# @cmd:
#
-# @arg1: the first argument
+# @arg1: the first argument
#
# @arg2: the second
# argument
and got
$ PYTHONPATH=/work/armbru/qemu/scripts python3 /work/armbru/qemu/tests/qapi-schema/test-qapi.py -d tests/qapi-schema doc-good.json
doc-good FAIL
--- tests/qapi-schema/doc-good.out
+++
@@ -149,12 +149,12 @@
== Another subsection
doc symbol=cmd
body=
-
- arg=arg1
-the first argument
+@arg1: the first argument
arg=arg2
the second
argument
+ arg=arg1
+
arg=arg3
feature=cmd-feat1
[...]
OK, more time in the oven with this one, and I will tackle it separately and later. Possibly as part of my sphinx-docs work I want to get to soon. We may drop it from this series to avoid holding it up.
(The FIXME again keeps me honest here ... !)
Thanks for the reviews!
--js