Hi On Tue, Mar 14, 2017 at 8:14 PM Markus Armbruster <arm...@redhat.com> wrote:
> Marc-André Lureau <marcandre.lur...@gmail.com> writes: > > > Hi > > > > On Mon, Mar 13, 2017 at 5:12 PM Markus Armbruster <arm...@redhat.com> > wrote: > > > >> Marc-André Lureau <marcandre.lur...@gmail.com> writes: > >> > >> > Hi > >> > > >> > On Mon, Mar 13, 2017 at 4:14 PM Markus Armbruster <arm...@redhat.com> > wrote: > >> > > >> >> Marc-André Lureau <marcandre.lur...@gmail.com> writes: > >> >> > >> >> > Hi > >> >> > > >> >> > On Mon, Mar 13, 2017 at 10:23 AM Markus Armbruster < > arm...@redhat.com > >> > > >> >> > 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: < > 20170125130308.16104-1-marcandre.lur...@redhat.com> > >> >> >> > 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. > > Doc comments that merely restate the type should be cleaned up. > > It's not just that, it's also the verbosity of the description that clutters the information you need. > Often you don't need > to > > read the documentation / description, you want to quickly check the > return > > type, and remind you the arguments. > > Point taken. > > A formal description of unbounded (and often excessive) length can't > serve that purpose, though. > In which case we are screwed anyway in any form > > A sufficiently condensed summaries just might. Perhaps names only, no > types. Certainly no more than a few. > > For instance, having > > -- Command: block-job-set-speed device speed > > instead of just > > -- Command: block-job-set-speed > > feels okay; the additional two words are technically redundant, but they > might occasionally serve someone as a reminder, and they're not > distracting. > In my generated pdf, using my type proposal (I use it daily), I have: block-job-set-speed (’device’: str , ’speed’: int ) Which I find useful. I am going to lack it, and in fact I'll probably maintain my own version of the document if we don't have that declaration form. At least for a while, until I get used to your type version eventually). It feels like bikeshedding at this point, and you are the maintainer, so I'll probably stop arguing, but it doesn't mean I am satisfied with your proposal. > But I feel > > -- Command: blockdev-mirror [job-id] device target [replaces] sync > [speed] [granularity] [buf-size] [on-source-error] > [on-target-error] [filter-node-name] > > is pushing it. > > So this begs the question which ones to omit when there are more than a > few. I'm afraid asking a stupid computer program to pick out > "important" arguments is asking for too much. For high-quality > summaries, we'd have to pick ourselves. > > Moreover, what to do for truly complex commands like blockdev-add? > Simply omitting all variant members is one option: > > -- Command: blockdev-add driver [node-name] [discard] [cache] > [read-only] [detect-zeroes] ... > > But what may work for blockdev-add need not work for other complex > commands. > > > 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. > > The worst offenders are actually commands such as blockdev-add and > block_set_io_throttle, unless we give up on the "reminder" mission for > them and merely add a reference to their (named) argument type. > -- Marc-André Lureau