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

[John Snow <jsnow@redhat.com>]

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