Re: cleanups with long-term benefits (was Re: [PATCH] schemas: Add vim modeline)

[Nir Soffer <nsoffer@redhat.com>]

On Mon, Aug 3, 2020 at 9:19 PM John Snow <jsnow@redhat.com> wrote:
>
> On 8/3/20 2:16 PM, Paolo Bonzini wrote:
> > On 03/08/20 20:10, John Snow wrote:
> >> Heresy:
> >>
> >> Docstrings could become part of the data format so they can be parsed,
> >> analyzed and validated. Parsers largely treat comments like non-semantic
> >> information and discard it. Round-trip parsers that preserve comments in
> >> any language are extremely rare.
> >>
> >> If the docstrings are relevant to the generator and aren't discardable,
> >> they should be fully-fledged data members.
> >>
> >> In a prototype I had for a YAML format, I just promoted docstrings
> >> directly to fields, so I could allow clients to query help text for
> >> individual commands.
> >
> > This would be actually a good idea, but somebody has to write the code.
> >   Each field's docstring should be attached to the field, however---no
> > parsing needed only looking at the tree.  Take a look at what Nir posted:
> >
> >> Here is the patch adding schema convertor from qemu "json" format to
> >> standard yaml:
> >> https://github.com/oVirt/vdsm/commit/e57b69e72987c0929b20306c454835b52b5eb7ee
> >>
> >> The current version of the new yaml based schema:
> >> https://github.com/oVirt/vdsm/blob/master/lib/vdsm/api/vdsm-api.yml
> >
> >
> >      VmDiskDevice: &VmDiskDevice
> >          added: '3.1'
> >          description: Properties of a VM disk device.
> >          name: VmDiskDevice
> >          properties:
> >          -   description: Indicates if writes are prohibited for the
> >                  device
> >              name: readonly
> >              type: boolean
> >
> >          -   description: The size of the disk (in bytes)
> >              name: apparentsize
> >              type: uint
> >
> > etc.
> >
> > Paolo
> >
>
> I was working on a small prototype that used something that looked like
> this; the "*opt" format was traded for "?opt", but otherwise:
>
>
> struct:
>    name: AudiodevPerDirectionOptions
>    doc: >
>      General audio backend options that are used for both
>      playback and recording.
>    since: '4.0'
>    members:
>
>      ?mixing-engine:

This optimizes for writing instead of reading.

    optional: true

Would be nicer to read, but more important is all the tools parsing
this schema in multiple languages that will have code like:

    def is_optional(node):
        return node.name.startswith("?")

Instead of :

   if node.optional:
       ...

Or maybe better:

    if node.required:

Because it seems that more nodes are optional, so focusing on the required
items will make the schema shorter and more clear.

>        type: bool
>        default: 'true'
>        since: '4.2'
>        doc: |
>          Use QEMU's mixing engine to mix all streams inside QEMU and
>          convert audio formats when not supported by the backend.

Using | is nicer than >-. Not sure what is the difference. In vdsm we don't use
anything and I think it causes trouble when indenting text.

>          When set to off, fixed-settings must be also off.
>
>      ?fixed-settings:
>        type: bool
>        default: 'true'

Why is the default a string and not the actual type?

>        doc: >-
>          Use fixed settings for host input/output.
>          When off, frequency, channels and format must not be specified.
>
>      ?frequency:
>        type: bool
>        default: '44100'
>        doc: >-
>          frequency to use when using fixed settings.
>
>      ?channels:
>        type: 'uint32'
>        default: 2

Here you use the real type, and this is nicer.

>        doc: >-
>          Number of channels when using fixed settings.
>
>      ?voices:
>        type: 'uint32'
>        default: 1
>        doc: "Number of voices to use."
>
>      ?format:
>        type: 'AudioFormat'
>        default: 's16'
>        doc: "Sample format to use when using fixed settings."
>
>      ?buffer-length:
>        type: 'uint32'
>        doc: 'The buffer length, in microseconds.'
>
>    features:
>      my-cool-feature:
>        since: '6.0'
>        doc: 'This is, no doubt, an extremely cool feature.'
>
>      my-bad-feature:
>        doc: 'This is a very bad feature. I am sorry for making it.'
>        since: '1.0'
>        deprecated: '5.9'

Good example :-)

>
>