Re: [Qemu-devel] [PATCH v2 01/11] qapi: add qapi2texi script

[Eric Blake]

On 10/28/2016 11:44 AM, Markus Armbruster wrote:
> Marc-André Lureau <address@hidden> writes:
> 
>> As the name suggests, the qapi2texi script converts JSON QAPI
>> description into a standalone texi file suitable for different target
>> formats.
>>
>> It parses the following kind of blocks with some little variations:
>>
>>   ##
>>   # = Section
>>   # == Subsection
>>   #
>>   # Some text foo with *emphasis*
>>   # 1. with a list
>>   # 2. like that
>>   #
>>   # And some code:
>>   # | $ echo foo
>>   # | <- do this
>>   # | -> get that

Backwards.  Pre-patch, qmp-commands.txt states:

> Also, the following notation is used to denote data flow:
> 
> -> data issued by the Client
> <- Server data response

so you want

-> do this
<- get that

>>   #
>>   ##

How much effort is it to get rid of the "little variations" and flag
documentation that does not match consistent patterns?  A tighter parser
that requires specific format, but also diagnoses where comments don't
meet that format, is probably going to be easier to maintain in the long
run (fewer special cases) even if it is more painful up front (more
tweaks to bring things into a consistent layout).


>>
>>   ##
>>   # @symbol
>>   #
>>   # Symbol body ditto ergo sum. Foo bar
>>   # baz ding.
>>   #
>>   # @arg: foo
>>   # @arg: #optional foo
>>   #
>>   # Returns: returns bla bla
>>   #
>>   #          Or bla blah
>>   #
>>   # Since: version
>>   # Notes: notes, comments can have
>>   #        - itemized list
>>   #        - like this
>>   #
>>   #        and continue...
>>   #
>>   # Example:
>>   #
>>   # <- { "execute": "quit" }
>>   # -> { "return": {} }

Again, backwards.

>>   #
>>   ##
>>
>> Thanks to the json declaration, it's able to give extra information
>> about the type of arguments and return value expected.
>>
>> Signed-off-by: Marc-André Lureau <address@hidden>
>> ---
>>  scripts/qapi.py        | 100 +++++++++++++++-
>>  scripts/qapi2texi.py   | 314 
>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>  docs/qapi-code-gen.txt |  44 +++++--
>>  3 files changed, 446 insertions(+), 12 deletions(-)
>>  create mode 100755 scripts/qapi2texi.py
> 
> My review will be easier to follow if you skip ahead qapi-code-gen.txt,
> then go to class QAPISchemaParser, then come back here.

Commenting on just a few points:

>> +            if key.startswith("@"):
>> +                key = key[1:].rstrip(':')
> 
> Treats the colon as optional.  I understand you're trying to be
> unintrusive, but since we're parsing comments already, we can make the
> parser do double duty and enforce consistent comment formatting.  But
> this isn't the place to discuss the format we want, review of
> qapi-code-gen.txt is.

Makes sense - since we are writing a parser, we can make it be strict
and pick one approved format, rather than allowing two separate formats;
the parser will point out what needs to be fixed, and we actually make
those fixes then rearrange the series so the fixes come first.

> 
> Taken together:
> 
> * The QAPI parser is hacked to accumulate and exfiltrate some comments
>   to the QAPIDoc parser.

I haven't closely looked at the code yet, just the review comments at
this point.  During the QAPI parse of the .json file, are we passing the
entire ##...## comment (of which there should be at most one per
top-level element of the .json file), and then letting QAPIDoc further
parse that comment block?  Or are you trying to combine both levels of
parsing into the same parser object?


> General remarks:
> 
> * I feel the only way to keep the API documentation in order as we
>   develop is to have the parser reject malformed API comments.

Agreed. Forcing docs to be well-written up front will pay benefits in
ease of maintenance and uniformity of style (it's a lot easier to
copy-and-paste a working style if that is the only style to copy from),
once we have converted existing documentation to be well-formed.

> 
> * The only way to create a robust and maintainable parser is to specify
>   the language *first* and *rigorously*.
> 
> * Stacking parsers is not a good idea.  Figuring out one parser's
>   accepted language is hard enough.

We do have the nice aspect that there is (should be) at most one ##...##
comment body per QAPI (pseudo-JSON) object, so the QAPI parse just needs
to find that comment block and associate it with everything else it
parsed.  Further parsing the documentation comment into sections for
output, as well as double-checking sanity aspects (such as no missing or
extra parameters) may require more coordination between the parsed QAPI
object and the code that parses the comments.


>> +++ b/docs/qapi-code-gen.txt
>> @@ -45,16 +45,13 @@ QAPI parser does not).  At present, there is no place 
>> where a QAPI
>>  schema requires the use of JSON numbers or null.
>>  
>>  Comments are allowed; anything between an unquoted # and the following
>> -newline is ignored.  Although there is not yet a documentation
>> -generator, a form of stylized comments has developed for consistently
>> -documenting details about an expression and when it was added to the
>> -schema.  The documentation is delimited between two lines of ##, then
>> -the first line names the expression, an optional overview is provided,
>> -then individual documentation about each member of 'data' is provided,
>> -and finally, a 'Since: x.y.z' tag lists the release that introduced
>> -the expression.  Optional members are tagged with the phrase
>> -'#optional', often with their default value; and extensions added
>> -after the expression was first released are also given a '(since
>> +newline is ignored.  The documentation is delimited between two lines
>> +of ##, then the first line names the expression, an optional overview
>> +is provided, then individual documentation about each member of 'data'
>> +is provided, and finally, a 'Since: x.y.z' tag lists the release that
>> +introduced the expression.  Optional members are tagged with the
>> +phrase '#optional', often with their default value; and extensions
>> +added after the expression was first released are also given a '(since
>>  x.y.z)' comment.  For example:

Okay, so you ARE requiring the grammar to pull out all lines between two
## markers, as well as looking for specific features to delineate
sections with the lines.  This text doesn't mention Return or Example,
so maybe it's worth further spelling it out, something like:


api_comment ::= double_hash expr_name [overviews] [members] \
                 [return] since [note] [examples] double_hash

along with semantic constraints.  For example, expr_name ::= name ':',
with the constraint that name matches the JSON description; members
would be a list of productions matching the JSON description, where each
member ::= '@' name ':' [#optional] freeform [member_since].

>>  
>>      ##
>> @@ -73,12 +70,39 @@ x.y.z)' comment.  For example:
>>      #           (Since 2.0)
>>      #
>>      # Since: 0.14.0
>> +    #
>> +    # Notes: You can also make a list:
>> +    #        - with items
>> +    #        - like this
>> +    #
>> +    # Example:
>> +    #
>> +    # <- { "execute": ... }
>> +    # -> { "return": ... }

Again, backwards (looks like copy and paste between the
qapi-code-gen.txt and the commit message).

>> +    #
>>      ##
>>      { 'struct': 'BlockStats',
>>        'data': {'*device': 'str', 'stats': 'BlockDeviceStats',
>>                 '*parent': 'BlockStats',
>>                 '*backing': 'BlockStats'} }
>>  
>> +It's also possible to create documentation sections, such as:
>> +
>> +    ##
>> +    # = Section
>> +    # == Subsection
>> +    #
>> +    # Some text foo with *emphasis*
>> +    # 1. with a list
>> +    # 2. like that
>> +    #
>> +    # And some code:
>> +    # | $ echo foo
>> +    # | <- do this
>> +    # | -> get that
>> +    #
>> +    ##
>> +
>>  The schema sets up a series of types, as well as commands and events
>>  that will use those types.  Forward references are allowed: the parser
>>  scans in two passes, where the first pass learns all type names, and
> 
> This may be good enough to guide users, but it's not rigorous enough to
> actually implement a parser.  The API doc language needs to be specified
> as a grammar.  If not here, then elsewhere.

I agree with the sentiment - a tighter description on exactly what is
valid vs. required, and where some of the validity is semantics based
(the comments must match the JSON description), can help us write a
parser that enforces those rules.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature