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

[Marc-André Lureau]

Hi

On Mon, Mar 13, 2017 at 5:12 PM Markus Armbruster <address@hidden> wrote:

> 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.
>

It is more natural and faster to read to me for commands and events for
example.  The verbose description is mixing description and sometime even
providing redundant information (ex: keys: array of KeyValue,  An array of
'KeyValue' elements...), slowing reading even more. Often you don't need to
read the documentation / description, you want to quickly check the return
type, and remind you the arguments.

struct/objects are more commonly declared with a line per member, so it
doesn't bother me as much.

I would appreciate if can have the declarative form for commands and events
at least. Other types are usually more complex or long, so that may clear
your concerns for the long declarations.
-- 
Marc-André Lureau