diff mbox series

[08/29] qapi: Use ':' after @argument in doc comments

Message ID 20200206173040.17337-9-peter.maydell@linaro.org
State Superseded
Headers show
Series Convert QAPI doc comments to generate rST instead of texinfo | expand

Commit Message

Peter Maydell Feb. 6, 2020, 5:30 p.m. UTC
Some qapi doc comments have forgotten the ':' after the
@argument, like this:

# @filename         Filename for the new image file
# @size             Size of the virtual disk in bytes

The result is that these are parsed as part of the body
text and appear as a run-on line:
  filename Filename for the new image file size Size of the virtual disk in bytes"
followed by
  filename: string
    Not documented
  size: int
    Not documented

in the 'Members' section.

Correct the formatting.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

---
 qapi/block-core.json | 236 +++++++++++++++++++++----------------------
 1 file changed, 118 insertions(+), 118 deletions(-)

-- 
2.20.1

Comments

Markus Armbruster Feb. 7, 2020, 9:28 a.m. UTC | #1
Peter Maydell <peter.maydell@linaro.org> writes:

> Some qapi doc comments have forgotten the ':' after the

> @argument, like this:

>

> # @filename         Filename for the new image file

> # @size             Size of the virtual disk in bytes

>

> The result is that these are parsed as part of the body

> text and appear as a run-on line:

>   filename Filename for the new image file size Size of the virtual disk in bytes"

> followed by

>   filename: string

>     Not documented

>   size: int

>     Not documented

>

> in the 'Members' section.


Easy error to make, and due to the "anything goes" nature of the doc
comment syntax, the QAPI generator won't help you.

>

> Correct the formatting.

>

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> ---

>  qapi/block-core.json | 236 +++++++++++++++++++++----------------------

>  1 file changed, 118 insertions(+), 118 deletions(-)

>

> diff --git a/qapi/block-core.json b/qapi/block-core.json

> index 372f35ee5f0..076a4a4808e 100644

> --- a/qapi/block-core.json

> +++ b/qapi/block-core.json

> @@ -3235,9 +3235,9 @@

>  ##

>  # @SshHostKeyCheckMode:

>  #

> -# @none             Don't check the host key at all

> -# @hash             Compare the host key with a given hash

> -# @known_hosts      Check the host key against the known_hosts file

> +# @none: Don't check the host key at all

> +# @hash: Compare the host key with a given hash

> +# @known_hosts: Check the host key against the known_hosts file

>  #

>  # Since: 2.12

>  ##

> @@ -3247,8 +3247,8 @@

>  ##

>  # @SshHostKeyCheckHashType:

>  #

> -# @md5              The given hash is an md5 hash

> -# @sha1             The given hash is an sha1 hash

> +# @md5: The given hash is an md5 hash

> +# @sha1: The given hash is an sha1 hash

>  #

>  # Since: 2.12

>  ##

> @@ -3258,8 +3258,8 @@

>  ##

>  # @SshHostKeyHash:

>  #

> -# @type             The hash algorithm used for the hash

> -# @hash             The expected hash value

> +# @type: The hash algorithm used for the hash

> +# @hash: The expected hash value

>  #

>  # Since: 2.12

>  ##

> @@ -4265,13 +4265,13 @@

>  #

>  # Driver specific image creation options for file.

>  #

> -# @filename         Filename for the new image file

> -# @size             Size of the virtual disk in bytes

> -# @preallocation    Preallocation mode for the new image (default: off;

> -#                   allowed values: off,

> -#                   falloc (if defined CONFIG_POSIX_FALLOCATE),

> -#                   full (if defined CONFIG_POSIX))

> -# @nocow            Turn off copy-on-write (valid only on btrfs; default: off)

> +# @filename: Filename for the new image file

> +# @size: Size of the virtual disk in bytes

> +# @preallocation: Preallocation mode for the new image (default: off;

> +#                 allowed values: off,

> +#                 falloc (if defined CONFIG_POSIX_FALLOCATE),

> +#                 full (if defined CONFIG_POSIX))

> +# @nocow: Turn off copy-on-write (valid only on btrfs; default: off)

>  #

>  # Since: 2.12

>  ##

> @@ -4286,12 +4286,12 @@

>  #

>  # Driver specific image creation options for gluster.

>  #

> -# @location         Where to store the new image file

> -# @size             Size of the virtual disk in bytes

> -# @preallocation    Preallocation mode for the new image (default: off;

> -#                   allowed values: off,

> -#                   falloc (if defined CONFIG_GLUSTERFS_FALLOCATE),

> -#                   full (if defined CONFIG_GLUSTERFS_ZEROFILL))

> +# @location: Where to store the new image file

> +# @size: Size of the virtual disk in bytes

> +# @preallocation: Preallocation mode for the new image (default: off;

> +#                 allowed values: off,

> +#                 falloc (if defined CONFIG_GLUSTERFS_FALLOCATE),

> +#                 full (if defined CONFIG_GLUSTERFS_ZEROFILL))

>  #

>  # Since: 2.12

>  ##

> @@ -4305,11 +4305,11 @@

>  #

>  # Driver specific image creation options for LUKS.

>  #

> -# @file             Node to create the image format on

> -# @size             Size of the virtual disk in bytes

> -# @preallocation    Preallocation mode for the new image

> -#                   (since: 4.2)

> -#                   (default: off; allowed values: off, metadata, falloc, full)

> +# @file: Node to create the image format on

> +# @size: Size of the virtual disk in bytes

> +# @preallocation: Preallocation mode for the new image

> +#                 (since: 4.2)

> +#                 (default: off; allowed values: off, metadata, falloc, full)

>  #

>  # Since: 2.12

>  ##

> @@ -4324,8 +4324,8 @@

>  #

>  # Driver specific image creation options for NFS.

>  #

> -# @location         Where to store the new image file

> -# @size             Size of the virtual disk in bytes

> +# @location: Where to store the new image file

> +# @size: Size of the virtual disk in bytes

>  #

>  # Since: 2.12

>  ##

> @@ -4338,9 +4338,9 @@

>  #

>  # Driver specific image creation options for parallels.

>  #

> -# @file             Node to create the image format on

> -# @size             Size of the virtual disk in bytes

> -# @cluster-size     Cluster size in bytes (default: 1 MB)

> +# @file: Node to create the image format on

> +# @size: Size of the virtual disk in bytes

> +# @cluster-size: Cluster size in bytes (default: 1 MB)

>  #

>  # Since: 2.12

>  ##

> @@ -4354,11 +4354,11 @@

>  #

>  # Driver specific image creation options for qcow.

>  #

> -# @file             Node to create the image format on

> -# @size             Size of the virtual disk in bytes

> -# @backing-file     File name of the backing file if a backing file

> -#                   should be used

> -# @encrypt          Encryption options if the image should be encrypted

> +# @file: Node to create the image format on

> +# @size: Size of the virtual disk in bytes

> +# @backing-file: File name of the backing file if a backing file

> +#                should be used

> +# @encrypt: Encryption options if the image should be encrypted

>  #

>  # Since: 2.12

>  ##

> @@ -4385,24 +4385,24 @@

>  #

>  # Driver specific image creation options for qcow2.

>  #

> -# @file             Node to create the image format on

> -# @data-file        Node to use as an external data file in which all guest

> -#                   data is stored so that only metadata remains in the qcow2

> -#                   file (since: 4.0)

> -# @data-file-raw    True if the external data file must stay valid as a

> -#                   standalone (read-only) raw image without looking at qcow2

> -#                   metadata (default: false; since: 4.0)

> -# @size             Size of the virtual disk in bytes

> -# @version          Compatibility level (default: v3)

> -# @backing-file     File name of the backing file if a backing file

> -#                   should be used

> -# @backing-fmt      Name of the block driver to use for the backing file

> -# @encrypt          Encryption options if the image should be encrypted

> -# @cluster-size     qcow2 cluster size in bytes (default: 65536)

> -# @preallocation    Preallocation mode for the new image (default: off;

> -#                   allowed values: off, falloc, full, metadata)

> -# @lazy-refcounts   True if refcounts may be updated lazily (default: off)

> -# @refcount-bits    Width of reference counts in bits (default: 16)

> +# @file: Node to create the image format on

> +# @data-file: Node to use as an external data file in which all guest

> +#             data is stored so that only metadata remains in the qcow2

> +#             file (since: 4.0)

> +# @data-file-raw: True if the external data file must stay valid as a

> +#                 standalone (read-only) raw image without looking at qcow2

> +#                 metadata (default: false; since: 4.0)

> +# @size: Size of the virtual disk in bytes

> +# @version: Compatibility level (default: v3)

> +# @backing-file: File name of the backing file if a backing file

> +#                should be used

> +# @backing-fmt: Name of the block driver to use for the backing file

> +# @encrypt: Encryption options if the image should be encrypted

> +# @cluster-size: qcow2 cluster size in bytes (default: 65536)

> +# @preallocation: Preallocation mode for the new image (default: off;

> +#                 allowed values: off, falloc, full, metadata)

> +# @lazy-refcounts: True if refcounts may be updated lazily (default: off)

> +# @refcount-bits: Width of reference counts in bits (default: 16)

>  #

>  # Since: 2.12

>  ##

> @@ -4425,13 +4425,13 @@

>  #

>  # Driver specific image creation options for qed.

>  #

> -# @file             Node to create the image format on

> -# @size             Size of the virtual disk in bytes

> -# @backing-file     File name of the backing file if a backing file

> -#                   should be used

> -# @backing-fmt      Name of the block driver to use for the backing file

> -# @cluster-size     Cluster size in bytes (default: 65536)

> -# @table-size       L1/L2 table size (in clusters)

> +# @file: Node to create the image format on

> +# @size: Size of the virtual disk in bytes

> +# @backing-file: File name of the backing file if a backing file

> +#                should be used

> +# @backing-fmt: Name of the block driver to use for the backing file

> +# @cluster-size: Cluster size in bytes (default: 65536)

> +# @table-size: L1/L2 table size (in clusters)

>  #

>  # Since: 2.12

>  ##

> @@ -4448,10 +4448,10 @@

>  #

>  # Driver specific image creation options for rbd/Ceph.

>  #

> -# @location         Where to store the new image file. This location cannot

> -#                   point to a snapshot.

> -# @size             Size of the virtual disk in bytes

> -# @cluster-size     RBD object size

> +# @location: Where to store the new image file. This location cannot

> +#            point to a snapshot.

> +# @size: Size of the virtual disk in bytes

> +# @cluster-size: RBD object size

>  #

>  # Since: 2.12

>  ##

> @@ -4499,23 +4499,23 @@

>  #

>  # Driver specific image creation options for VMDK.

>  #

> -# @file         Where to store the new image file. This refers to the image

> -#               file for monolithcSparse and streamOptimized format, or the

> -#               descriptor file for other formats.

> -# @size         Size of the virtual disk in bytes

> -# @extents      Where to store the data extents. Required for monolithcFlat,

> -#               twoGbMaxExtentSparse and twoGbMaxExtentFlat formats. For

> -#               monolithicFlat, only one entry is required; for

> -#               twoGbMaxExtent* formats, the number of entries required is

> -#               calculated as extent_number = virtual_size / 2GB. Providing

> -#               more extents than will be used is an error.

> -# @subformat    The subformat of the VMDK image. Default: "monolithicSparse".

> -# @backing-file The path of backing file. Default: no backing file is used.

> -# @adapter-type The adapter type used to fill in the descriptor. Default: ide.

> -# @hwversion    Hardware version. The meaningful options are "4" or "6".

> -#               Default: "4".

> -# @zeroed-grain Whether to enable zeroed-grain feature for sparse subformats.

> -#               Default: false.

> +# @file: Where to store the new image file. This refers to the image

> +#        file for monolithcSparse and streamOptimized format, or the

> +#        descriptor file for other formats.

> +# @size: Size of the virtual disk in bytes

> +# @extents: Where to store the data extents. Required for monolithcFlat,

> +#           twoGbMaxExtentSparse and twoGbMaxExtentFlat formats. For

> +#           monolithicFlat, only one entry is required; for

> +#           twoGbMaxExtent* formats, the number of entries required is

> +#           calculated as extent_number = virtual_size / 2GB. Providing

> +#           more extents than will be used is an error.

> +# @subformat: The subformat of the VMDK image. Default: "monolithicSparse".

> +# @backing-file: The path of backing file. Default: no backing file is used.

> +# @adapter-type: The adapter type used to fill in the descriptor. Default: ide.

> +# @hwversion: Hardware version. The meaningful options are "4" or "6".

> +#             Default: "4".

> +# @zeroed-grain: Whether to enable zeroed-grain feature for sparse subformats.

> +#                Default: false.

>  #

>  # Since: 4.0

>  ##

> @@ -4533,9 +4533,9 @@

>  ##

>  # @SheepdogRedundancyType:

>  #

> -# @full             Create a fully replicated vdi with x copies

> -# @erasure-coded    Create an erasure coded vdi with x data strips and

> -#                   y parity strips

> +# @full: Create a fully replicated vdi with x copies

> +# @erasure-coded: Create an erasure coded vdi with x data strips and

> +#                 y parity strips

>  #

>  # Since: 2.12

>  ##

> @@ -4545,7 +4545,7 @@

>  ##

>  # @SheepdogRedundancyFull:

>  #

> -# @copies           Number of copies to use (between 1 and 31)

> +# @copies: Number of copies to use (between 1 and 31)

>  #

>  # Since: 2.12

>  ##

> @@ -4555,8 +4555,8 @@

>  ##

>  # @SheepdogRedundancyErasureCoded:

>  #

> -# @data-strips      Number of data strips to use (one of {2,4,8,16})

> -# @parity-strips    Number of parity strips to use (between 1 and 15)

> +# @data-strips: Number of data strips to use (one of {2,4,8,16})

> +# @parity-strips: Number of parity strips to use (between 1 and 15)

>  #

>  # Since: 2.12

>  ##

> @@ -4580,13 +4580,13 @@

>  #

>  # Driver specific image creation options for Sheepdog.

>  #

> -# @location         Where to store the new image file

> -# @size             Size of the virtual disk in bytes

> -# @backing-file     File name of a base image

> -# @preallocation    Preallocation mode for the new image (default: off;

> -#                   allowed values: off, full)

> -# @redundancy       Redundancy of the image

> -# @object-size      Object size of the image

> +# @location: Where to store the new image file

> +# @size: Size of the virtual disk in bytes

> +# @backing-file: File name of a base image

> +# @preallocation: Preallocation mode for the new image (default: off;

> +#                 allowed values: off, full)

> +# @redundancy: Redundancy of the image

> +# @object-size: Object size of the image

>  #

>  # Since: 2.12

>  ##

> @@ -4603,8 +4603,8 @@

>  #

>  # Driver specific image creation options for SSH.

>  #

> -# @location         Where to store the new image file

> -# @size             Size of the virtual disk in bytes

> +# @location: Where to store the new image file

> +# @size: Size of the virtual disk in bytes

>  #

>  # Since: 2.12

>  ##

> @@ -4617,10 +4617,10 @@

>  #

>  # Driver specific image creation options for VDI.

>  #

> -# @file             Node to create the image format on

> -# @size             Size of the virtual disk in bytes

> -# @preallocation    Preallocation mode for the new image (default: off;

> -#                   allowed values: off, metadata)

> +# @file: Node to create the image format on

> +# @size: Size of the virtual disk in bytes

> +# @preallocation: Preallocation mode for the new image (default: off;

> +#                 allowed values: off, metadata)

>  #

>  # Since: 2.12

>  ##

> @@ -4645,17 +4645,17 @@

>  #

>  # Driver specific image creation options for vhdx.

>  #

> -# @file             Node to create the image format on

> -# @size             Size of the virtual disk in bytes

> -# @log-size         Log size in bytes, must be a multiple of 1 MB

> -#                   (default: 1 MB)

> -# @block-size       Block size in bytes, must be a multiple of 1 MB and not

> -#                   larger than 256 MB (default: automatically choose a block

> -#                   size depending on the image size)

> -# @subformat        vhdx subformat (default: dynamic)

> -# @block-state-zero Force use of payload blocks of type 'ZERO'. Non-standard,

> -#                   but default.  Do not set to 'off' when using 'qemu-img

> -#                   convert' with subformat=dynamic.

> +# @file: Node to create the image format on

> +# @size: Size of the virtual disk in bytes

> +# @log-size: Log size in bytes, must be a multiple of 1 MB

> +#            (default: 1 MB)

> +# @block-size: Block size in bytes, must be a multiple of 1 MB and not

> +#              larger than 256 MB (default: automatically choose a block

> +#              size depending on the image size)

> +# @subformat: vhdx subformat (default: dynamic)

> +# @block-state-zero: Force use of payload blocks of type 'ZERO'. Non-standard,

> +#                    but default.  Do not set to 'off' when using 'qemu-img

> +#                    convert' with subformat=dynamic.

>  #

>  # Since: 2.12

>  ##

> @@ -4683,12 +4683,12 @@

>  #

>  # Driver specific image creation options for vpc (VHD).

>  #

> -# @file             Node to create the image format on

> -# @size             Size of the virtual disk in bytes

> -# @subformat        vhdx subformat (default: dynamic)

> -# @force-size       Force use of the exact byte size instead of rounding to the

> -#                   next size that can be represented in CHS geometry

> -#                   (default: false)

> +# @file: Node to create the image format on

> +# @size: Size of the virtual disk in bytes

> +# @subformat: vhdx subformat (default: dynamic)

> +# @force-size: Force use of the exact byte size instead of rounding to the

> +#              next size that can be represented in CHS geometry

> +#              (default: false)

>  #

>  # Since: 2.12

>  ##

> @@ -4703,7 +4703,7 @@

>  #

>  # Options for creating an image format on a given node.

>  #

> -# @driver           block driver to create the image format

> +# @driver: block driver to create the image format

>  #

>  # Since: 2.12

>  ##


Loses the visual alignment.  I'm okay with that, but the folks who took
the trouble to align the text may have different ideas.  Cc'ing Kevin
and Max.

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Max Reitz Feb. 7, 2020, 9:33 a.m. UTC | #2
On 07.02.20 10:28, Markus Armbruster wrote:

[...]

> Loses the visual alignment.  I'm okay with that, but the folks who took

> the trouble to align the text may have different ideas.  Cc'ing Kevin

> and Max.


I certainly don’t mind.

Max
Kevin Wolf Feb. 7, 2020, 10:24 a.m. UTC | #3
Am 07.02.2020 um 10:28 hat Markus Armbruster geschrieben:
> Peter Maydell <peter.maydell@linaro.org> writes:

> > @@ -4703,7 +4703,7 @@

> >  #

> >  # Options for creating an image format on a given node.

> >  #

> > -# @driver           block driver to create the image format

> > +# @driver: block driver to create the image format

> >  #

> >  # Since: 2.12

> >  ##

> 

> Loses the visual alignment.  I'm okay with that, but the folks who took

> the trouble to align the text may have different ideas.  Cc'ing Kevin

> and Max.


I think the documentation is much easier to parse visually with aligned
text as it makes both the option name and the whole part of the comment
that documents options stand out clearly.

Of course, "It is the QEMU coding style." would trump everything, but as
long as there isn't a style guide that requires a wall of text without
spaces and alignment, I'd prefer to leave at least those aligned texts
in place that we have.

Kevin
Peter Maydell Feb. 7, 2020, 11:05 a.m. UTC | #4
On Fri, 7 Feb 2020 at 10:24, Kevin Wolf <kwolf@redhat.com> wrote:
>

> Am 07.02.2020 um 10:28 hat Markus Armbruster geschrieben:

> > Peter Maydell <peter.maydell@linaro.org> writes:

> > > @@ -4703,7 +4703,7 @@

> > >  #

> > >  # Options for creating an image format on a given node.

> > >  #

> > > -# @driver           block driver to create the image format

> > > +# @driver: block driver to create the image format

> > >  #

> > >  # Since: 2.12

> > >  ##

> >

> > Loses the visual alignment.  I'm okay with that, but the folks who took

> > the trouble to align the text may have different ideas.  Cc'ing Kevin

> > and Max.

>

> I think the documentation is much easier to parse visually with aligned

> text as it makes both the option name and the whole part of the comment

> that documents options stand out clearly.

>

> Of course, "It is the QEMU coding style." would trump everything, but as

> long as there isn't a style guide that requires a wall of text without

> spaces and alignment, I'd prefer to leave at least those aligned texts

> in place that we have.


So, the other way to handle this would be to say "the @foo: can have
an arbitrary amount of whitespace after it", and have the doc comment
parser strip out that many characters of extra whitespace there and
on the subsequent lines. The downside is that then you would have no
way of having a comment for an argument which started with rST markup
that required leading whitespace. I think this pretty much would just
mean that you can't start an argument description with a blockquote,
so we don't lose much, but there is a difference currently between:

@arg:    In the current parser this is a blockquote
         Blockquote line 2

      But this is a non-blockquoted line still in @arg's description

and

@arg: This is not blockquoted, it's just a line
      So is this
      and this

I can make the parser work the other way if people prefer that though
(and then the first example above would become a syntax error because
the 3rd line would be unexpectedly de-indented).

thanks
-- PMM
Markus Armbruster Feb. 7, 2020, 2:43 p.m. UTC | #5
Peter Maydell <peter.maydell@linaro.org> writes:

> On Fri, 7 Feb 2020 at 10:24, Kevin Wolf <kwolf@redhat.com> wrote:

>>

>> Am 07.02.2020 um 10:28 hat Markus Armbruster geschrieben:

>> > Peter Maydell <peter.maydell@linaro.org> writes:

>> > > @@ -4703,7 +4703,7 @@

>> > >  #

>> > >  # Options for creating an image format on a given node.

>> > >  #

>> > > -# @driver           block driver to create the image format

>> > > +# @driver: block driver to create the image format

>> > >  #

>> > >  # Since: 2.12

>> > >  ##

>> >

>> > Loses the visual alignment.  I'm okay with that, but the folks who took

>> > the trouble to align the text may have different ideas.  Cc'ing Kevin

>> > and Max.

>>

>> I think the documentation is much easier to parse visually with aligned

>> text as it makes both the option name and the whole part of the comment

>> that documents options stand out clearly.

>>

>> Of course, "It is the QEMU coding style." would trump everything, but as

>> long as there isn't a style guide that requires a wall of text without

>> spaces and alignment, I'd prefer to leave at least those aligned texts

>> in place that we have.

>

> So, the other way to handle this would be to say "the @foo: can have

> an arbitrary amount of whitespace after it", and have the doc comment

> parser strip out that many characters of extra whitespace there and

> on the subsequent lines. The downside is that then you would have no

> way of having a comment for an argument which started with rST markup

> that required leading whitespace. I think this pretty much would just

> mean that you can't start an argument description with a blockquote,

> so we don't lose much, but there is a difference currently between:

>

> @arg:    In the current parser this is a blockquote

>          Blockquote line 2

>

>       But this is a non-blockquoted line still in @arg's description

>

> and

>

> @arg: This is not blockquoted, it's just a line

>       So is this

>       and this

>

> I can make the parser work the other way if people prefer that though

> (and then the first example above would become a syntax error because

> the 3rd line would be unexpectedly de-indented).


Let me ignore rST details for a bit.

The prevailing schema style looks like this:

    # @file: Node to create the image format on
    # @size: Size of the virtual disk in bytes
    # @log-size: Log size in bytes, must be a multiple of 1 MB
    #            (default: 1 MB)
    # @block-size: Block size in bytes, must be a multiple of 1 MB and not
    #              larger than 256 MB (default: automatically choose a block
    #              size depending on the image size)
    # @subformat: vhdx subformat (default: dynamic)
    # @block-state-zero: Force use of payload blocks of type 'ZERO'. Non-standard,
    #                    but default.  Do not set to 'off' when using 'qemu-img
    #                    convert' with subformat=dynamic.

Peter's patch converts to it.  Can't fault him for converting to the
prevailing style.

Trouble is the prevailing style is ugly, and can waste massive amounts
of screen real estate when both the identifier and the explaining text
are long.

block*.json's style looks like this:

    # @file:             Node to create the image format on
    # @size:             Size of the virtual disk in bytes
    # @log-size:         Log size in bytes, must be a multiple of 1 MB
    #                    (default: 1 MB)
    # @block-size:       Block size in bytes, must be a multiple of 1 MB and not
    #                    larger than 256 MB (default: automatically choose a block
    #                    size depending on the image size)
    # @subformat:        vhdx subformat (default: dynamic)
    # @block-state-zero: Force use of payload blocks of type 'ZERO'. Non-standard,
    #                    but default.  Do not set to 'off' when using 'qemu-img
    #                    convert' with subformat=dynamic.

I dislike this style, too.  It's less ugly, until you add a longer
member.  Then you either accept inconsistent indentation, or reindent
all the other members.  Blech.

Here's a style I'd dislike less:

    # @file: Node to create the image format on
    #
    # @size: Size of the virtual disk in bytes
    #
    # @log-size: Log size in bytes, must be a multiple of 1 MB
    #     (default: 1 MB)
    #
    # @block-size: Block size in bytes, must be a multiple of 1 MB and not
    #     larger than 256 MB (default: automatically choose a block
    #     size depending on the image size)
    #
    # @subformat: vhdx subformat (default: dynamic)
    #
    # @block-state-zero: Force use of payload blocks of type 'ZERO'.
    #     Non-standard, but default.  Do not set to 'off' when using
    #     'qemu-img convert' with subformat=dynamic.

Or maybe even

    # @file:
    # Node to create the image format on
    #
    # @size:
    # Size of the virtual disk in bytes
    #
    # @log-size:
    # Log size in bytes, must be a multiple of 1 MB (default: 1 MB)
    #
    # @block-size:
    # Block size in bytes, must be a multiple of 1 MB and not larger
    # than 256 MB (default: automatically choose a block size depending
    # on the image size)
    #
    # @subformat:
    # vhdx subformat (default: dynamic)
    #
    # @block-state-zero:
    # Force use of payload blocks of type 'ZERO'.  Non-standard, but
    # default.  Do not set to 'off' when using 'qemu-img convert' with
    # subformat=dynamic.

With both these styles, member names stand out reasonably well, and I
don't have to fiddle with indentation when adding or removing members.
With the second one, I don't have to fiddle with indentation at all.

The second one might be the better fit for rST, but that's for Peter to
judge.
Max Reitz Feb. 7, 2020, 3:01 p.m. UTC | #6
On 07.02.20 15:43, Markus Armbruster wrote:

[...]

>     # @file:

>     # Node to create the image format on

>     #

>     # @size:

>     # Size of the virtual disk in bytes

>     #

>     # @log-size:

>     # Log size in bytes, must be a multiple of 1 MB (default: 1 MB)

>     #

>     # @block-size:

>     # Block size in bytes, must be a multiple of 1 MB and not larger

>     # than 256 MB (default: automatically choose a block size depending

>     # on the image size)

>     #

>     # @subformat:

>     # vhdx subformat (default: dynamic)

>     #

>     # @block-state-zero:

>     # Force use of payload blocks of type 'ZERO'.  Non-standard, but

>     # default.  Do not set to 'off' when using 'qemu-img convert' with

>     # subformat=dynamic.


FWIW, I like this one.

Max
Peter Maydell Feb. 7, 2020, 3:24 p.m. UTC | #7
On Fri, 7 Feb 2020 at 14:43, Markus Armbruster <armbru@redhat.com> wrote:


> Here's a style I'd dislike less:

>

>     # @file: Node to create the image format on

>     #

>     # @size: Size of the virtual disk in bytes

>     #

>     # @log-size: Log size in bytes, must be a multiple of 1 MB

>     #     (default: 1 MB)

>     #

>     # @block-size: Block size in bytes, must be a multiple of 1 MB and not

>     #     larger than 256 MB (default: automatically choose a block

>     #     size depending on the image size)

>     #

>     # @subformat: vhdx subformat (default: dynamic)

>     #

>     # @block-state-zero: Force use of payload blocks of type 'ZERO'.

>     #     Non-standard, but default.  Do not set to 'off' when using

>     #     'qemu-img convert' with subformat=dynamic.


The problem with this one is that there's no way for the
doc-comment parser to know how far lines 2,3... are
supposed to be indented. Unlike the block-quote issue, this
is a real problem, because it's not possible to distinguish:

# @foo: - Here's a bulleted list
#         Line 2 of the list item should indent to match the first
# @bar: - A one item list
#       A line not in the list

which is the kind of thing that will show up in real-world
usage. (Unless you wanted to say "always 4-space indent" or something,
which I think would tend to result in a lot of accidental
over-indentation and unintended blockquotes in the output.)

> Or maybe even

>

>     # @file:

>     # Node to create the image format on

>     #

>     # @size:

>     # Size of the virtual disk in bytes

>     #

>     # @log-size:

>     # Log size in bytes, must be a multiple of 1 MB (default: 1 MB)

>     #

>     # @block-size:

>     # Block size in bytes, must be a multiple of 1 MB and not larger

>     # than 256 MB (default: automatically choose a block size depending

>     # on the image size)

>     #

>     # @subformat:

>     # vhdx subformat (default: dynamic)

>     #

>     # @block-state-zero:

>     # Force use of payload blocks of type 'ZERO'.  Non-standard, but

>     # default.  Do not set to 'off' when using 'qemu-img convert' with

>     # subformat=dynamic.


Conveniently this patchset already supports this format :-)
You can write either

# @foo: bar
#       baz
#         indented

or
# @foo:
# bar
# baz
#   indented

and they'll come out to the same thing (the parser.py code
sends the same doc strings to the rST visitor).

thanks
-- PMM
Kevin Wolf Feb. 7, 2020, 3:40 p.m. UTC | #8
Am 07.02.2020 um 16:01 hat Max Reitz geschrieben:
> On 07.02.20 15:43, Markus Armbruster wrote:

> 

> [...]

> 

> >     # @file:

> >     # Node to create the image format on

> >     #

> >     # @size:

> >     # Size of the virtual disk in bytes

> >     #

> >     # @log-size:

> >     # Log size in bytes, must be a multiple of 1 MB (default: 1 MB)

> >     #

> >     # @block-size:

> >     # Block size in bytes, must be a multiple of 1 MB and not larger

> >     # than 256 MB (default: automatically choose a block size depending

> >     # on the image size)

> >     #

> >     # @subformat:

> >     # vhdx subformat (default: dynamic)

> >     #

> >     # @block-state-zero:

> >     # Force use of payload blocks of type 'ZERO'.  Non-standard, but

> >     # default.  Do not set to 'off' when using 'qemu-img convert' with

> >     # subformat=dynamic.

> 

> FWIW, I like this one.


Looks like a workable compromise to me, too.

Kevin
Markus Armbruster Feb. 8, 2020, 7:54 a.m. UTC | #9
Peter Maydell <peter.maydell@linaro.org> writes:

> On Fri, 7 Feb 2020 at 14:43, Markus Armbruster <armbru@redhat.com> wrote:

>

>

>> Here's a style I'd dislike less:

[...]
>>     # @file:

>>     # Node to create the image format on

>>     #

>>     # @size:

>>     # Size of the virtual disk in bytes

>>     #

>>     # @log-size:

>>     # Log size in bytes, must be a multiple of 1 MB (default: 1 MB)

>>     #

>>     # @block-size:

>>     # Block size in bytes, must be a multiple of 1 MB and not larger

>>     # than 256 MB (default: automatically choose a block size depending

>>     # on the image size)

>>     #

>>     # @subformat:

>>     # vhdx subformat (default: dynamic)

>>     #

>>     # @block-state-zero:

>>     # Force use of payload blocks of type 'ZERO'.  Non-standard, but

>>     # default.  Do not set to 'off' when using 'qemu-img convert' with

>>     # subformat=dynamic.

>

> Conveniently this patchset already supports this format :-)

> You can write either

>

> # @foo: bar

> #       baz

> #         indented

>

> or

> # @foo:

> # bar

> # baz

> #   indented

>

> and they'll come out to the same thing (the parser.py code

> sends the same doc strings to the rST visitor).


If we enforce the second format in the QAPI schema parser, we save
ourselves the trouble of normalizing the first format to the second one.
We also promote more uniform style.
Peter Maydell Feb. 8, 2020, 1:22 p.m. UTC | #10
On Sat, 8 Feb 2020 at 07:54, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:


> > Conveniently this patchset already supports this format :-)

> > You can write either

> >

> > # @foo: bar

> > #       baz

> > #         indented

> >

> > or

> > # @foo:

> > # bar

> > # baz

> > #   indented

> >

> > and they'll come out to the same thing (the parser.py code

> > sends the same doc strings to the rST visitor).

>

> If we enforce the second format in the QAPI schema parser, we save

> ourselves the trouble of normalizing the first format to the second one.

> We also promote more uniform style.


You also end up requiring
# @foo:
# A short one-line description.

because it's the first style that makes this valid syntax:
# @foo: A short one-line description

And you're suggesting a big upheaval in doc comment style, because
lots of the existing doc comment syntax uses the first version.
I would really strongly prefer to not have "convert to supporting rST"
also mean "and we have to touch every single JSON doc comment
to convert away from a commonly used style that nobody has
complained about in the past, which doesn't compromise the ability
to include rST markup in the doc comment, and which was easy to
support in the doc generator". This patchset is already quite large
and has a lot of updates to QAPI doc comments. The one that makes
the widest set of changes (patch 9/29) is bigger than I'd like and I think
an important thing that makes it viable is that you can check with
git show that it really is just changing whitespace, not even line breaks.
Some of the style changes you're proposing would be much harder
to verify as safe and touch much more of the JSON. If you'd like
to do those I have no objection, but I really really don't want to
tangle that up with the already large amount of work involved
in transitioning away from texi to rST.

thanks
-- PMM
diff mbox series

Patch

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 372f35ee5f0..076a4a4808e 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3235,9 +3235,9 @@ 
 ##
 # @SshHostKeyCheckMode:
 #
-# @none             Don't check the host key at all
-# @hash             Compare the host key with a given hash
-# @known_hosts      Check the host key against the known_hosts file
+# @none: Don't check the host key at all
+# @hash: Compare the host key with a given hash
+# @known_hosts: Check the host key against the known_hosts file
 #
 # Since: 2.12
 ##
@@ -3247,8 +3247,8 @@ 
 ##
 # @SshHostKeyCheckHashType:
 #
-# @md5              The given hash is an md5 hash
-# @sha1             The given hash is an sha1 hash
+# @md5: The given hash is an md5 hash
+# @sha1: The given hash is an sha1 hash
 #
 # Since: 2.12
 ##
@@ -3258,8 +3258,8 @@ 
 ##
 # @SshHostKeyHash:
 #
-# @type             The hash algorithm used for the hash
-# @hash             The expected hash value
+# @type: The hash algorithm used for the hash
+# @hash: The expected hash value
 #
 # Since: 2.12
 ##
@@ -4265,13 +4265,13 @@ 
 #
 # Driver specific image creation options for file.
 #
-# @filename         Filename for the new image file
-# @size             Size of the virtual disk in bytes
-# @preallocation    Preallocation mode for the new image (default: off;
-#                   allowed values: off,
-#                   falloc (if defined CONFIG_POSIX_FALLOCATE),
-#                   full (if defined CONFIG_POSIX))
-# @nocow            Turn off copy-on-write (valid only on btrfs; default: off)
+# @filename: Filename for the new image file
+# @size: Size of the virtual disk in bytes
+# @preallocation: Preallocation mode for the new image (default: off;
+#                 allowed values: off,
+#                 falloc (if defined CONFIG_POSIX_FALLOCATE),
+#                 full (if defined CONFIG_POSIX))
+# @nocow: Turn off copy-on-write (valid only on btrfs; default: off)
 #
 # Since: 2.12
 ##
@@ -4286,12 +4286,12 @@ 
 #
 # Driver specific image creation options for gluster.
 #
-# @location         Where to store the new image file
-# @size             Size of the virtual disk in bytes
-# @preallocation    Preallocation mode for the new image (default: off;
-#                   allowed values: off,
-#                   falloc (if defined CONFIG_GLUSTERFS_FALLOCATE),
-#                   full (if defined CONFIG_GLUSTERFS_ZEROFILL))
+# @location: Where to store the new image file
+# @size: Size of the virtual disk in bytes
+# @preallocation: Preallocation mode for the new image (default: off;
+#                 allowed values: off,
+#                 falloc (if defined CONFIG_GLUSTERFS_FALLOCATE),
+#                 full (if defined CONFIG_GLUSTERFS_ZEROFILL))
 #
 # Since: 2.12
 ##
@@ -4305,11 +4305,11 @@ 
 #
 # Driver specific image creation options for LUKS.
 #
-# @file             Node to create the image format on
-# @size             Size of the virtual disk in bytes
-# @preallocation    Preallocation mode for the new image
-#                   (since: 4.2)
-#                   (default: off; allowed values: off, metadata, falloc, full)
+# @file: Node to create the image format on
+# @size: Size of the virtual disk in bytes
+# @preallocation: Preallocation mode for the new image
+#                 (since: 4.2)
+#                 (default: off; allowed values: off, metadata, falloc, full)
 #
 # Since: 2.12
 ##
@@ -4324,8 +4324,8 @@ 
 #
 # Driver specific image creation options for NFS.
 #
-# @location         Where to store the new image file
-# @size             Size of the virtual disk in bytes
+# @location: Where to store the new image file
+# @size: Size of the virtual disk in bytes
 #
 # Since: 2.12
 ##
@@ -4338,9 +4338,9 @@ 
 #
 # Driver specific image creation options for parallels.
 #
-# @file             Node to create the image format on
-# @size             Size of the virtual disk in bytes
-# @cluster-size     Cluster size in bytes (default: 1 MB)
+# @file: Node to create the image format on
+# @size: Size of the virtual disk in bytes
+# @cluster-size: Cluster size in bytes (default: 1 MB)
 #
 # Since: 2.12
 ##
@@ -4354,11 +4354,11 @@ 
 #
 # Driver specific image creation options for qcow.
 #
-# @file             Node to create the image format on
-# @size             Size of the virtual disk in bytes
-# @backing-file     File name of the backing file if a backing file
-#                   should be used
-# @encrypt          Encryption options if the image should be encrypted
+# @file: Node to create the image format on
+# @size: Size of the virtual disk in bytes
+# @backing-file: File name of the backing file if a backing file
+#                should be used
+# @encrypt: Encryption options if the image should be encrypted
 #
 # Since: 2.12
 ##
@@ -4385,24 +4385,24 @@ 
 #
 # Driver specific image creation options for qcow2.
 #
-# @file             Node to create the image format on
-# @data-file        Node to use as an external data file in which all guest
-#                   data is stored so that only metadata remains in the qcow2
-#                   file (since: 4.0)
-# @data-file-raw    True if the external data file must stay valid as a
-#                   standalone (read-only) raw image without looking at qcow2
-#                   metadata (default: false; since: 4.0)
-# @size             Size of the virtual disk in bytes
-# @version          Compatibility level (default: v3)
-# @backing-file     File name of the backing file if a backing file
-#                   should be used
-# @backing-fmt      Name of the block driver to use for the backing file
-# @encrypt          Encryption options if the image should be encrypted
-# @cluster-size     qcow2 cluster size in bytes (default: 65536)
-# @preallocation    Preallocation mode for the new image (default: off;
-#                   allowed values: off, falloc, full, metadata)
-# @lazy-refcounts   True if refcounts may be updated lazily (default: off)
-# @refcount-bits    Width of reference counts in bits (default: 16)
+# @file: Node to create the image format on
+# @data-file: Node to use as an external data file in which all guest
+#             data is stored so that only metadata remains in the qcow2
+#             file (since: 4.0)
+# @data-file-raw: True if the external data file must stay valid as a
+#                 standalone (read-only) raw image without looking at qcow2
+#                 metadata (default: false; since: 4.0)
+# @size: Size of the virtual disk in bytes
+# @version: Compatibility level (default: v3)
+# @backing-file: File name of the backing file if a backing file
+#                should be used
+# @backing-fmt: Name of the block driver to use for the backing file
+# @encrypt: Encryption options if the image should be encrypted
+# @cluster-size: qcow2 cluster size in bytes (default: 65536)
+# @preallocation: Preallocation mode for the new image (default: off;
+#                 allowed values: off, falloc, full, metadata)
+# @lazy-refcounts: True if refcounts may be updated lazily (default: off)
+# @refcount-bits: Width of reference counts in bits (default: 16)
 #
 # Since: 2.12
 ##
@@ -4425,13 +4425,13 @@ 
 #
 # Driver specific image creation options for qed.
 #
-# @file             Node to create the image format on
-# @size             Size of the virtual disk in bytes
-# @backing-file     File name of the backing file if a backing file
-#                   should be used
-# @backing-fmt      Name of the block driver to use for the backing file
-# @cluster-size     Cluster size in bytes (default: 65536)
-# @table-size       L1/L2 table size (in clusters)
+# @file: Node to create the image format on
+# @size: Size of the virtual disk in bytes
+# @backing-file: File name of the backing file if a backing file
+#                should be used
+# @backing-fmt: Name of the block driver to use for the backing file
+# @cluster-size: Cluster size in bytes (default: 65536)
+# @table-size: L1/L2 table size (in clusters)
 #
 # Since: 2.12
 ##
@@ -4448,10 +4448,10 @@ 
 #
 # Driver specific image creation options for rbd/Ceph.
 #
-# @location         Where to store the new image file. This location cannot
-#                   point to a snapshot.
-# @size             Size of the virtual disk in bytes
-# @cluster-size     RBD object size
+# @location: Where to store the new image file. This location cannot
+#            point to a snapshot.
+# @size: Size of the virtual disk in bytes
+# @cluster-size: RBD object size
 #
 # Since: 2.12
 ##
@@ -4499,23 +4499,23 @@ 
 #
 # Driver specific image creation options for VMDK.
 #
-# @file         Where to store the new image file. This refers to the image
-#               file for monolithcSparse and streamOptimized format, or the
-#               descriptor file for other formats.
-# @size         Size of the virtual disk in bytes
-# @extents      Where to store the data extents. Required for monolithcFlat,
-#               twoGbMaxExtentSparse and twoGbMaxExtentFlat formats. For
-#               monolithicFlat, only one entry is required; for
-#               twoGbMaxExtent* formats, the number of entries required is
-#               calculated as extent_number = virtual_size / 2GB. Providing
-#               more extents than will be used is an error.
-# @subformat    The subformat of the VMDK image. Default: "monolithicSparse".
-# @backing-file The path of backing file. Default: no backing file is used.
-# @adapter-type The adapter type used to fill in the descriptor. Default: ide.
-# @hwversion    Hardware version. The meaningful options are "4" or "6".
-#               Default: "4".
-# @zeroed-grain Whether to enable zeroed-grain feature for sparse subformats.
-#               Default: false.
+# @file: Where to store the new image file. This refers to the image
+#        file for monolithcSparse and streamOptimized format, or the
+#        descriptor file for other formats.
+# @size: Size of the virtual disk in bytes
+# @extents: Where to store the data extents. Required for monolithcFlat,
+#           twoGbMaxExtentSparse and twoGbMaxExtentFlat formats. For
+#           monolithicFlat, only one entry is required; for
+#           twoGbMaxExtent* formats, the number of entries required is
+#           calculated as extent_number = virtual_size / 2GB. Providing
+#           more extents than will be used is an error.
+# @subformat: The subformat of the VMDK image. Default: "monolithicSparse".
+# @backing-file: The path of backing file. Default: no backing file is used.
+# @adapter-type: The adapter type used to fill in the descriptor. Default: ide.
+# @hwversion: Hardware version. The meaningful options are "4" or "6".
+#             Default: "4".
+# @zeroed-grain: Whether to enable zeroed-grain feature for sparse subformats.
+#                Default: false.
 #
 # Since: 4.0
 ##
@@ -4533,9 +4533,9 @@ 
 ##
 # @SheepdogRedundancyType:
 #
-# @full             Create a fully replicated vdi with x copies
-# @erasure-coded    Create an erasure coded vdi with x data strips and
-#                   y parity strips
+# @full: Create a fully replicated vdi with x copies
+# @erasure-coded: Create an erasure coded vdi with x data strips and
+#                 y parity strips
 #
 # Since: 2.12
 ##
@@ -4545,7 +4545,7 @@ 
 ##
 # @SheepdogRedundancyFull:
 #
-# @copies           Number of copies to use (between 1 and 31)
+# @copies: Number of copies to use (between 1 and 31)
 #
 # Since: 2.12
 ##
@@ -4555,8 +4555,8 @@ 
 ##
 # @SheepdogRedundancyErasureCoded:
 #
-# @data-strips      Number of data strips to use (one of {2,4,8,16})
-# @parity-strips    Number of parity strips to use (between 1 and 15)
+# @data-strips: Number of data strips to use (one of {2,4,8,16})
+# @parity-strips: Number of parity strips to use (between 1 and 15)
 #
 # Since: 2.12
 ##
@@ -4580,13 +4580,13 @@ 
 #
 # Driver specific image creation options for Sheepdog.
 #
-# @location         Where to store the new image file
-# @size             Size of the virtual disk in bytes
-# @backing-file     File name of a base image
-# @preallocation    Preallocation mode for the new image (default: off;
-#                   allowed values: off, full)
-# @redundancy       Redundancy of the image
-# @object-size      Object size of the image
+# @location: Where to store the new image file
+# @size: Size of the virtual disk in bytes
+# @backing-file: File name of a base image
+# @preallocation: Preallocation mode for the new image (default: off;
+#                 allowed values: off, full)
+# @redundancy: Redundancy of the image
+# @object-size: Object size of the image
 #
 # Since: 2.12
 ##
@@ -4603,8 +4603,8 @@ 
 #
 # Driver specific image creation options for SSH.
 #
-# @location         Where to store the new image file
-# @size             Size of the virtual disk in bytes
+# @location: Where to store the new image file
+# @size: Size of the virtual disk in bytes
 #
 # Since: 2.12
 ##
@@ -4617,10 +4617,10 @@ 
 #
 # Driver specific image creation options for VDI.
 #
-# @file             Node to create the image format on
-# @size             Size of the virtual disk in bytes
-# @preallocation    Preallocation mode for the new image (default: off;
-#                   allowed values: off, metadata)
+# @file: Node to create the image format on
+# @size: Size of the virtual disk in bytes
+# @preallocation: Preallocation mode for the new image (default: off;
+#                 allowed values: off, metadata)
 #
 # Since: 2.12
 ##
@@ -4645,17 +4645,17 @@ 
 #
 # Driver specific image creation options for vhdx.
 #
-# @file             Node to create the image format on
-# @size             Size of the virtual disk in bytes
-# @log-size         Log size in bytes, must be a multiple of 1 MB
-#                   (default: 1 MB)
-# @block-size       Block size in bytes, must be a multiple of 1 MB and not
-#                   larger than 256 MB (default: automatically choose a block
-#                   size depending on the image size)
-# @subformat        vhdx subformat (default: dynamic)
-# @block-state-zero Force use of payload blocks of type 'ZERO'. Non-standard,
-#                   but default.  Do not set to 'off' when using 'qemu-img
-#                   convert' with subformat=dynamic.
+# @file: Node to create the image format on
+# @size: Size of the virtual disk in bytes
+# @log-size: Log size in bytes, must be a multiple of 1 MB
+#            (default: 1 MB)
+# @block-size: Block size in bytes, must be a multiple of 1 MB and not
+#              larger than 256 MB (default: automatically choose a block
+#              size depending on the image size)
+# @subformat: vhdx subformat (default: dynamic)
+# @block-state-zero: Force use of payload blocks of type 'ZERO'. Non-standard,
+#                    but default.  Do not set to 'off' when using 'qemu-img
+#                    convert' with subformat=dynamic.
 #
 # Since: 2.12
 ##
@@ -4683,12 +4683,12 @@ 
 #
 # Driver specific image creation options for vpc (VHD).
 #
-# @file             Node to create the image format on
-# @size             Size of the virtual disk in bytes
-# @subformat        vhdx subformat (default: dynamic)
-# @force-size       Force use of the exact byte size instead of rounding to the
-#                   next size that can be represented in CHS geometry
-#                   (default: false)
+# @file: Node to create the image format on
+# @size: Size of the virtual disk in bytes
+# @subformat: vhdx subformat (default: dynamic)
+# @force-size: Force use of the exact byte size instead of rounding to the
+#              next size that can be represented in CHS geometry
+#              (default: false)
 #
 # Since: 2.12
 ##
@@ -4703,7 +4703,7 @@ 
 #
 # Options for creating an image format on a given node.
 #
-# @driver           block driver to create the image format
+# @driver: block driver to create the image format
 #
 # Since: 2.12
 ##