Re: [Qemu-devel] [PATCH for-2.9 17/47] qapi: The #optional tag is redundant, drop

[Markus Armbruster]

Eric Blake  writes:

> On 03/13/2017 01:18 AM, Markus Armbruster wrote:
>> We traditionally mark optional members #optional in the doc comment.
>> Before commit 3313b61, this was entirely manual.
>> 
>> Commit 3313b61 added some automation because its qapi2texi.py relied
>> on #optional to determine whether a member is optional.  This is no
>> longer the case since the previous commit: the only thing qapi2texi.py
>> still does with #optional is stripping it out.  We still reject bogus
>> qapi-schema.json and six places for qga/qapi-schema.json.
>> 
>> Thus, you can't actually rely on #optional to see whether something is
>> optional.  Yet we still make people add it manually.  That's just
>> busy-work.
>
> Yay! Let the computer do the work for us!
>
>> 
>> Drop the code to check, fix up and strip out #optional, along with all
>> instances of #optional.  To keep it out, add code to reject it, to be
>> dropped again once the dust settles.
>> 
>> No change to generated documentation.
>> 
>> Signed-off-by: Markus Armbruster 
>> ---
>
>> @@ -150,10 +148,10 @@ For example:
>>  #
>>  # Statistics of a virtual block device or a block backing device.
>>  #
>> -# @device: #optional If the stats are for a virtual block device, the name
>> -#  corresponding to the virtual block device.
>> +# @device: If the stats are for a virtual block device, the name
>> +# corresponding to the virtual block device.
>
> This loses the hanging indentation in the example, but I don't see you
> making that change in the actual .json files.  It shouldn't matter in
> the long run, and is certainly easier if the way you generated this
> patch was with sed scripts (where computing correct hanging indentation
> after rewrapping is a lot harder than omitting it).

Obvious sed job.

>  I don't have any
> strong opinions about the change (less typing, but slightly harder to
> visually see that the following lines belong to the same parameter doc,
> if you don't have blank lines between distinct parameter docs).  I'm not
> even sure if it you want to call it out in the commit message as an
> intentional reformat, particularly since you didn't do it everywhere.

I'll touch up this patch to change qapi-code-gen.txt exactly like the
schema.

>> +++ b/qapi-schema.json
>> @@ -150,10 +150,10 @@
>>  #
>>  # @fdname: file descriptor name previously passed via 'getfd' command
>>  #
>> -# @skipauth: #optional whether to skip authentication. Only applies
>> +# @skipauth: whether to skip authentication. Only applies
>>  #to "vnc" and "spice" protocols
>>  #
>> -# @tls: #optional whether to perform TLS. Only applies to the "spice"
>> +# @tls: whether to perform TLS. Only applies to the "spice"
>>  #   protocol
>
> Again, whitespace changes shouldn't affect generated output, so
> rewrapping lines like this would be more busy-work than necessary, even
> though this particular example would now fit on one line.
>
>> @@ -667,45 +667,45 @@
>>  #
> ...
>>  #
>> -# @setup-time: #optional amount of setup time in milliseconds _before_ the
>> +# @setup-time: amount of setup time in milliseconds _before_ the
>>  #iterations begin but _after_ the QMP command is issued. This is 
>> designed
>>  #to provide an accounting of any activities (such as RDMA pinning) 
>> which
>>  #may be expensive, but do not actually occur during the iterative
>>  #migration rounds themselves. (since 1.6)
>
> Here's another place where wrapping now looks odd (short, followed by
> multiple longer lines).  Again, the effort of rewrapping lines is not
> worth the churn (and it's actually easier to read diffs that _don't_
> reflow text).  So I'll just overlook wrapping oddities in the rest of
> the patch, as inconsequential to the end result.

A sufficiently clever Emacs macro could give us more pleasantly wrapped
lines, at the cost of a messier diff.  I'll experiment if time permits.

> Reviewed-by: Eric Blake 

Thanks!

Re: [Qemu-devel] [PATCH for-2.9 17/47] qapi: The #optional tag is redundant, drop

[Markus Armbruster]

Eric Blake  writes:

> On 03/13/2017 01:18 AM, Markus Armbruster wrote:
>> We traditionally mark optional members #optional in the doc comment.
>> Before commit 3313b61, this was entirely manual.
>> 
>> Commit 3313b61 added some automation because its qapi2texi.py relied
>> on #optional to determine whether a member is optional.  This is no
>> longer the case since the previous commit: the only thing qapi2texi.py
>> still does with #optional is stripping it out.  We still reject bogus
>> qapi-schema.json and six places for qga/qapi-schema.json.
>> 
>> Thus, you can't actually rely on #optional to see whether something is
>> optional.  Yet we still make people add it manually.  That's just
>> busy-work.
>> 
>> Drop the code to check, fix up and strip out #optional, along with all
>> instances of #optional.  To keep it out, add code to reject it, to be
>> dropped again once the dust settles.
>> 
>> No change to generated documentation.
>> 
>> Signed-off-by: Markus Armbruster 
>> ---
>>  docs/qapi-code-gen.txt  |  16 +-
>>  docs/writing-qmp-commands.txt   |   4 +-
>>  qapi-schema.json| 378 
>>  qapi/block-core.json| 418 
>> ++--
>
> You'll have to rebase this on master, due to ...
>
>
>> @@ -2349,17 +2349,17 @@
>>  #
>>  # @volume:  Name of the Archipelago volume image
>>  #
>> -# @mport:   #optional The port number on which mapperd is
>> +# @mport:   The port number on which mapperd is
>>  #   listening. This is optional
>>  #   and if not specified, QEMU will make Archipelago
>>  #   use the default port (1001).
>>  #
>
> ...commit e32ccbc killing broken archipelago

Yes.  Thanks for the heads-up.

Re: [Qemu-devel] [PATCH for-2.9 17/47] qapi: The #optional tag is redundant, drop

[Eric Blake]

On 03/13/2017 01:18 AM, Markus Armbruster wrote:
> We traditionally mark optional members #optional in the doc comment.
> Before commit 3313b61, this was entirely manual.
> 
> Commit 3313b61 added some automation because its qapi2texi.py relied
> on #optional to determine whether a member is optional.  This is no
> longer the case since the previous commit: the only thing qapi2texi.py
> still does with #optional is stripping it out.  We still reject bogus
> qapi-schema.json and six places for qga/qapi-schema.json.
> 
> Thus, you can't actually rely on #optional to see whether something is
> optional.  Yet we still make people add it manually.  That's just
> busy-work.
> 
> Drop the code to check, fix up and strip out #optional, along with all
> instances of #optional.  To keep it out, add code to reject it, to be
> dropped again once the dust settles.
> 
> No change to generated documentation.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  docs/qapi-code-gen.txt  |  16 +-
>  docs/writing-qmp-commands.txt   |   4 +-
>  qapi-schema.json| 378 
>  qapi/block-core.json| 418 
> ++--

You'll have to rebase this on master, due to ...


> @@ -2349,17 +2349,17 @@
>  #
>  # @volume:  Name of the Archipelago volume image
>  #
> -# @mport:   #optional The port number on which mapperd is
> +# @mport:   The port number on which mapperd is
>  #   listening. This is optional
>  #   and if not specified, QEMU will make Archipelago
>  #   use the default port (1001).
>  #

...commit e32ccbc killing broken archipelago

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH for-2.9 17/47] qapi: The #optional tag is redundant, drop

[Eric Blake]

On 03/13/2017 01:18 AM, Markus Armbruster wrote:
> We traditionally mark optional members #optional in the doc comment.
> Before commit 3313b61, this was entirely manual.
> 
> Commit 3313b61 added some automation because its qapi2texi.py relied
> on #optional to determine whether a member is optional.  This is no
> longer the case since the previous commit: the only thing qapi2texi.py
> still does with #optional is stripping it out.  We still reject bogus
> qapi-schema.json and six places for qga/qapi-schema.json.
> 
> Thus, you can't actually rely on #optional to see whether something is
> optional.  Yet we still make people add it manually.  That's just
> busy-work.

Yay! Let the computer do the work for us!

> 
> Drop the code to check, fix up and strip out #optional, along with all
> instances of #optional.  To keep it out, add code to reject it, to be
> dropped again once the dust settles.
> 
> No change to generated documentation.
> 
> Signed-off-by: Markus Armbruster 
> ---

> @@ -150,10 +148,10 @@ For example:
>  #
>  # Statistics of a virtual block device or a block backing device.
>  #
> -# @device: #optional If the stats are for a virtual block device, the name
> -#  corresponding to the virtual block device.
> +# @device: If the stats are for a virtual block device, the name
> +# corresponding to the virtual block device.

This loses the hanging indentation in the example, but I don't see you
making that change in the actual .json files.  It shouldn't matter in
the long run, and is certainly easier if the way you generated this
patch was with sed scripts (where computing correct hanging indentation
after rewrapping is a lot harder than omitting it).  I don't have any
strong opinions about the change (less typing, but slightly harder to
visually see that the following lines belong to the same parameter doc,
if you don't have blank lines between distinct parameter docs).  I'm not
even sure if it you want to call it out in the commit message as an
intentional reformat, particularly since you didn't do it everywhere.

> +++ b/qapi-schema.json
> @@ -150,10 +150,10 @@
>  #
>  # @fdname: file descriptor name previously passed via 'getfd' command
>  #
> -# @skipauth: #optional whether to skip authentication. Only applies
> +# @skipauth: whether to skip authentication. Only applies
>  #to "vnc" and "spice" protocols
>  #
> -# @tls: #optional whether to perform TLS. Only applies to the "spice"
> +# @tls: whether to perform TLS. Only applies to the "spice"
>  #   protocol

Again, whitespace changes shouldn't affect generated output, so
rewrapping lines like this would be more busy-work than necessary, even
though this particular example would now fit on one line.

> @@ -667,45 +667,45 @@
>  #
...
>  #
> -# @setup-time: #optional amount of setup time in milliseconds _before_ the
> +# @setup-time: amount of setup time in milliseconds _before_ the
>  #iterations begin but _after_ the QMP command is issued. This is 
> designed
>  #to provide an accounting of any activities (such as RDMA pinning) 
> which
>  #may be expensive, but do not actually occur during the iterative
>  #migration rounds themselves. (since 1.6)

Here's another place where wrapping now looks odd (short, followed by
multiple longer lines).  Again, the effort of rewrapping lines is not
worth the churn (and it's actually easier to read diffs that _don't_
reflow text).  So I'll just overlook wrapping oddities in the rest of
the patch, as inconsequential to the end result.

Reviewed-by: Eric Blake 

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



signature.asc
Description: OpenPGP digital signature