Re: [Qemu-devel] [PATCH for-2.9 00/47] qapi: Put type information back into QMP documentation

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> Hi
>
> On Mon, Mar 13, 2017 at 4:14 PM Markus Armbruster <address@hidden> wrote:
>
>> Marc-André Lureau <address@hidden> writes:
>>
>> > Hi
>> >
>> > On Mon, Mar 13, 2017 at 10:23 AM Markus Armbruster <address@hidden>
>> > wrote:
>> >
>> >> I'm proposing this is 2.9 because it fixes a documentation regression.
>> >> It affects only documentation; generated C code is unchanged except
>> >> for the removal of trailing space in PATCH 46.
>> >>
>> >> Based on my qapi-next branch, which contains Marc-André's PATCH 1/2.
>> >>
>> >> Marc-André's work to merge qmp-commands.txt and qmp-events.txt into
>> >> the QAPI schema and generate their replacements from the schema
>> >> (commit b6af8ea..56e8bdd) was a big step forward.  As committed, it
>> >> also was a step back: the documentation lost information on JSON
>> >> types, because I didn't like Marc-André's patch to add it.  He
>> >> reposted it for further review afterwards:
>> >>
>> >>     Subject: [PATCH 0/2] qapi2texi: add type information
>> >>     Message-Id: <address@hidden>
>> >>     https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg05432.html
>> >>
>> >> His PATCH 1/2 is a straightforward cleanup.  His PATCH 2/2 adds type
>> >> descriptions in a new formal language to the generated documentation.
>> >> Quoting the commit message:
>> >>
>> >>     Array types have the following syntax: type[]. Ex: str[].
>> >>
>> >>     - Struct, commands and events use the following members syntax:
>> >>
>> >>       { 'member': type, ('foo': str), ... }
>> >>
>> >>     Optional members are under parentheses.
>> >>
>> >>     A structure with a base type will have 'BaseStruct +' prepended.
>> >>
>> >>     - Alternates use the following syntax:
>> >>
>> >>       [ 'foo': type, 'bar': type, ... ]
>> >>
>> >>     - Simple unions use the following syntax:
>> >>
>> >>       { 'type': str, 'data': 'type' = [ 'foo': type, 'bar': type... ] }
>> >>
>> >>     - Flat unions use the following syntax:
>> >>
>> >>       BaseStruct + 'discriminator' = [ 'foo': type, 'bar': type... ]
>> >>
>> >> End quote.  Looks like this in generated documentation:
>> >>
>> >>  -- Event: VNC_CONNECTED {'server': VncServerInfo, 'client':
>> >>           VncBasicInfo}
>> >>
>> >>      Emitted when a VNC client establishes a connection
>> >>      ''server''
>> >>           server information
>> >>      ''client''
>> >>           client information
>> >>
>> >>      Note: This event is emitted before any authentication takes place,
>> >>      thus the authentication ID is not provided
>> >> [...]
>> >>
>> >>  -- Struct: VncServerInfo VncBasicInfo + {('auth': str)}
>> >>
>> >>      The network connection information for server
>> >>      ''auth'' (optional)
>> >>           authentication method used for the plain (non-websocket) VNC
>> >>           server
>> >>
>> >>      Since: 2.1
>> >>
>> >>  -- Simple Union: SocketAddress { 'type': str, 'data': 'type' = ['inet':
>> >>           InetSocketAddress, 'unix': UnixSocketAddress, 'vsock':
>> >>           VsockSocketAddress, 'fd': String] }
>> >>
>> >>      Captures the address of a socket, which could also be a named file
>> >>      descriptor
>> >>
>> >>      Since: 1.3
>> >>
>> >> Here's my counter-proposal: instead of inventing a formal language,
>> >> fix the natural language documentation to actually mention *all*
>> >> members, and add type information in a plain, easy-to-understand way.
>> >> Looks like this:
>> >>
>> >>  -- Event: VNC_CONNECTED
>> >>
>> >>      Emitted when a VNC client establishes a connection
>> >>
>> >>      Arguments:
>> >>      'server: VncServerInfo'
>> >>           server information
>> >>      'client: VncBasicInfo'
>> >>           client information
>> >>
>> >>      Note: This event is emitted before any authentication takes place,
>> >>      thus the authentication ID is not provided
>> >> [...]
>> >>
>> >>  -- Object: VncServerInfo
>> >>
>> >>      The network connection information for server
>> >>
>> >>      Members:
>> >>      'auth: string' (optional)
>> >>           authentication method used for the plain (non-websocket) VNC
>> >>           server
>> >>      The members of 'VncBasicInfo'
>> >>
>> >>      Since: 2.1
>> >>
>> >>  -- Object: SocketAddress
>> >>
>> >>      Captures the address of a socket, which could also be a named file
>> >>      descriptor
>> >>
>> >>      Members:
>> >>      'type'
>> >>           One of "inet", "unix", "vsock", "fd"
>> >>      'data: InetSocketAddress' when 'type' is "inet"
>> >>      'data: UnixSocketAddress' when 'type' is "unix"
>> >>      'data: VsockSocketAddress' when 'type' is "vsock"
>> >>      'data: String' when 'type' is "fd"
>> >>
>> >>      Since: 1.3
>> >>
>> >>
>> > I like both, to me they serve different purposes. I like to have a short
>> > overview / signature and then a more detailed documentation for each
>> field.
>>
>> I sympathize with the argument.  Unfortunately, the "short" signatures
>> are anything but for real-world QAPI:
>>
>
> That's a worse case, a regular case is more readable.

There are readable cases, but there are plenty of cases that plainly
aren't.

102 out of 472 signatures don't count because they're empty.

Roughly half the non-empty signatures fit on a single line.  That's short.

A bit under a third take two lines.  I guess that's still short enough.

More than one in six signatures is three lines or more.

>                                                       And it is still
> useful anyway since the common members would be listed first.

Whatever comes first in signatures comes first in the table of members,
too.  The names are easier to spot there, because they're all on the
left.

Compare

 -- Simple Union: SocketAddress { 'type': str, 'data': 'type' = ['inet':
          InetSocketAddress, 'unix': UnixSocketAddress, 'vsock':
          VsockSocketAddress, 'fd': String] }

     Captures the address of a socket, which could also be a named file
     descriptor

     Since: 1.3

to

 -- Object: SocketAddress

     Captures the address of a socket, which could also be a named file
     descriptor

     Members:
     'type'
          One of "inet", "unix", "vsock", "fd"
     'data: InetSocketAddress' when 'type' is "inet"
     'data: UnixSocketAddress' when 'type' is "unix"
     'data: VsockSocketAddress' when 'type' is "vsock"
     'data: String' when 'type' is "fd"

     Since: 1.3

In my opinion, the three lines of signature add nothing but noise to the
six lines of member table.