From: Markus Armbruster <armbru@redhat.com>
To: John Snow <jsnow@redhat.com>
Cc: Michael Roth <michael.roth@amd.com>,
Cleber Rosa <crosa@redhat.com>, Eric Blake <eblake@redhat.com>,
qemu-devel@nongnu.org, Eduardo Habkost <ehabkost@redhat.com>
Subject: Re: [PATCH v3 06/13] qapi/parser: remove FIXME comment from _append_body_line
Date: Thu, 30 Sep 2021 10:47:08 +0200 [thread overview]
Message-ID: <87y27eqwkz.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <20210929194428.1038496-7-jsnow@redhat.com> (John Snow's message of "Wed, 29 Sep 2021 15:44:21 -0400")
John Snow <jsnow@redhat.com> writes:
> True, we do not check the validity of this symbol -- but we don't check
> the validity of definition names during parse, either -- that happens
> later, during the expr check. I don't want to introduce a dependency on
> expr.py:check_name_str here and introduce a cycle.
>
> Instead, rest assured that a documentation block is required for each
> definition. This requirement uses the names of each section to ensure
> that we fulfilled this requirement.
>
> e.g., let's say that block-core.json has a comment block for
> "Snapshot!Info" by accident. We'll see this error message:
>
> In file included from ../../qapi/block.json:8:
> ../../qapi/block-core.json: In struct 'SnapshotInfo':
> ../../qapi/block-core.json:38: documentation comment is for 'Snapshot!Info'
>
> That's a pretty decent error message.
>
> Now, let's say that we actually mangle it twice, identically:
>
> ../../qapi/block-core.json: In struct 'Snapshot!Info':
> ../../qapi/block-core.json:38: struct has an invalid name
>
> That's also pretty decent. If we forget to fix it in both places, we'll
> just be back to the first error.
>
> Therefore, let's just drop this FIXME and adjust the error message to
> not imply a more thorough check than is actually performed.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> scripts/qapi/parser.py | 6 ++++--
> tests/qapi-schema/doc-empty-symbol.err | 2 +-
> 2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index 2f93a752f66..52748e8e462 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -558,9 +558,11 @@ def _append_body_line(self, line):
> raise QAPIParseError(
> self._parser, "extra whitespace around symbol declaration")
> self.symbol = line[1:-1]
> - # FIXME invalid names other than the empty string aren't flagged
> + # Invalid names are not checked here, but the name provided MUST
> + # match the following definition, which *is* validated.
> if not self.symbol:
> - raise QAPIParseError(self._parser, "invalid name")
> + raise QAPIParseError(
> + self._parser, "doc symbol name cannot be blank")
"blank" feels like "empty or just whitespace" to me. We actually mean
"empty".
What about "name required after @"?
> elif self.symbol:
> # This is a definition documentation block
> name = line.split(' ', 1)[0]
> diff --git a/tests/qapi-schema/doc-empty-symbol.err b/tests/qapi-schema/doc-empty-symbol.err
> index 81b90e882a7..a4981ee28ea 100644
> --- a/tests/qapi-schema/doc-empty-symbol.err
> +++ b/tests/qapi-schema/doc-empty-symbol.err
> @@ -1 +1 @@
> -doc-empty-symbol.json:4:1: invalid name
> +doc-empty-symbol.json:4:1: doc symbol name cannot be blank
next prev parent reply other threads:[~2021-09-30 8:49 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-29 19:44 [PATCH v3 00/13] qapi: static typing conversion, pt5b John Snow
2021-09-29 19:44 ` [PATCH v3 01/13] qapi/pylintrc: ignore 'consider-using-f-string' warning John Snow
2021-09-29 19:44 ` [PATCH v3 02/13] qapi/gen: use dict.items() to iterate over _modules John Snow
2021-09-29 19:44 ` [PATCH v3 03/13] qapi/parser: fix unused check_args_section arguments John Snow
2021-09-29 19:44 ` [PATCH v3 04/13] qapi: Add spaces after symbol declaration for consistency John Snow
2021-09-29 19:44 ` [PATCH v3 05/13] qapi/parser: improve detection of '@symbol:' preface John Snow
2021-09-30 8:42 ` Markus Armbruster
2021-09-30 17:43 ` John Snow
2021-09-29 19:44 ` [PATCH v3 06/13] qapi/parser: remove FIXME comment from _append_body_line John Snow
2021-09-30 8:47 ` Markus Armbruster [this message]
2021-09-30 17:20 ` John Snow
2021-09-29 19:44 ` [PATCH v3 07/13] qapi/parser: Simplify _end_section() John Snow
2021-09-29 19:44 ` [PATCH v3 08/13] qapi/parser: Introduce NullSection John Snow
2021-09-30 9:34 ` Markus Armbruster
2021-09-30 16:59 ` John Snow
2021-09-29 19:44 ` [PATCH v3 09/13] qapi/parser: add import cycle workaround John Snow
2021-09-30 9:45 ` Markus Armbruster
2021-09-30 17:11 ` John Snow
2021-09-29 19:44 ` [PATCH v3 10/13] qapi/parser: add type hint annotations (QAPIDoc) John Snow
2021-09-29 19:44 ` [PATCH v3 11/13] qapi/parser: enable mypy checks John Snow
2021-09-29 19:44 ` [PATCH v3 12/13] qapi/parser: Silence too-few-public-methods warning John Snow
2021-09-29 19:44 ` [PATCH v3 13/13] qapi/parser: enable pylint checks John Snow
2021-09-30 10:08 ` [PATCH v3 00/13] qapi: static typing conversion, pt5b Markus Armbruster
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87y27eqwkz.fsf@dusky.pond.sub.org \
--to=armbru@redhat.com \
--cc=crosa@redhat.com \
--cc=eblake@redhat.com \
--cc=ehabkost@redhat.com \
--cc=jsnow@redhat.com \
--cc=michael.roth@amd.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).