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

[John Snow]

On 8/3/20 3:54 PM, Nir Soffer wrote:
> 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.

Following a "path of least resistance" from the existing QAPI language, 
clearly carrying over the '*optional' syntax.

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

I believe when I wrote this example I was trying to highlight the 
different space consumption styles for the purposes of demonstrating 
what it would do to Sphinx document generation support.

ultimately, there's not really a way to enforce one or the other style 
post-parse.

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

I'm going to be honest: I forget. I was playing around with the idea of 
documenting defaults for the purposes of documentation, but not 
necessarily for performing the actual code generation of those defaults.

I believe I specified this field as a string in my grammar and `5` would 
get promoted to "5", but `true` caused a type error.

Doing something in a type-safe way seemed ... harder. So I didn't.

> >         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 :-)
>
> >