Eric Blake <ebl...@redhat.com> writes:
> On 03/13/2017 01:18 AM, Markus Armbruster wrote:
>> Since we added the documentation generator in commit 3313b61, doc
>> comments are mandatory. That's a very good idea for a schema that
>> needs to be documented, but has proven to be annoying for testing.
>
> As I've found out while rebasing my JSON output visitor as well as
> patches to allow anonymous bases to flat unions. Thanks, this will help me!
>
>>
>> Make doc comments optional again, but add a new directive
>>
>> { 'pragma': { 'doc-required': true } }
>>
>> to let a QAPI schema require them.
>
> I like it. It's extensible to other pragmas, as well; and reading
> ahead, it looks like you did a good job of flagging unknown pragmas.
>
>>
>> Require documentation in the schemas we actually want documented:
>> qapi-schema.json and qga/qapi-schema.json.
>>
>> We could probably make qapi2texi.py cope with incomplete
>> documentation, but for now, simply make it refuse to run unless the
>> schema has 'doc-required': true.
>
> I'd rather fail early on our non-documented stuff then risk broken
> corner cases; so I think you made the right decision.
I figure making qapi2texi.py robust wouldn't be hard, but decided not to
try for 2.9.
>>
>> Signed-off-by: Markus Armbruster <arm...@redhat.com>
>> ---
>> docs/qapi-code-gen.txt | 40
>> +++++++++++++++++++++++++-------------
>
>> @@ -277,6 +280,11 @@ class QAPISchemaParser(object):
>> "Value of 'include' must be a
>> string")
>> self._include(include, info, os.path.dirname(abs_fname),
>> previously_included)
>> + elif "pragma" in expr:
>> + if len(expr) != 1:
>> + raise QAPISemError(info, "Invalid 'pragma' directive")
>
> You may also want to check that you have an actual dictionary; otherwise...
>
>> + for name, value in expr['pragma'].iteritems():
>
> calling .iteritems() can lead to some funky python messages. For
> example, tweaking qapi-schema.json to use
>
> { 'pragma': [ { 'doc-required': true } ] }
>
> =>
>
> $ make
> GEN qmp-commands.h
> Traceback (most recent call last):
> File "/home/eblake/qemu/scripts/qapi-commands.py", line 317, in <module>
> schema = QAPISchema(input_file)
> File "/home/eblake/qemu/scripts/qapi.py", line 1496, in __init__
> parser = QAPISchemaParser(open(fname, "r"))
> File "/home/eblake/qemu/scripts/qapi.py", line 286, in __init__
> for name, value in expr['pragma'].iteritems():
> AttributeError: 'list' object has no attribute 'iteritems'
> Makefile:431: recipe for target 'qmp-commands.h' failed
You're right. Need to decide whether to fix it in a respin or on top.
> Not the end of the world, but we've done a nice job elsewhere of
> avoiding cryptic python traces.
>
>> + global doc_required
>> + if name == 'doc-required':
>> + if not isinstance(value, bool):
>> + raise QAPISemError(info,
>> + "Pragma 'doc-required' must be boolean")
>> + doc_required = value
>
> No testsuite coverage of this message?
Not yet, i.e. you're right, test coverage is advisable even for exotic
stuff that's expected not to change like pragma.
> If you decide to not bother, or to defer error message(s) cleanup to
> followups (especially since we are running short on time for fixing the
> actual doc regression from going into 2.9), then using this patch as-is
> can have:
>
> Reviewed-by: Eric Blake <ebl...@redhat.com>
Thanks!
> Of course, if you spin a v2 that actually addresses my concerns, it
> probably will be more involved than something that can trivially keep my
> R-b (in part because you'll probably also want a testsuite addition to
> cover any new error message...).
Possible :)
