Re: [PATCH] schemas: Add vim modeline

[Nir Soffer <nsoffer@redhat.com>]

On Thu, Jul 30, 2020 at 12:38 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Thu, Jul 30, 2020 at 11:07:26AM +0200, Markus Armbruster wrote:
> > Andrea Bolognani <abologna@redhat.com> writes:
> >
> > > The various schemas included in QEMU use a JSON-based format which
> > > is, however, strictly speaking not valid JSON.
> > >
> > > As a consequence, when vim tries to apply syntax highlight rules
> > > for JSON (as guessed from the file name), the result is an unreadable
> > > mess which mostly consist of red markers pointing out supposed errors
> > > in, well, pretty much everything.
> > >
> > > Using Python syntax highlighting produces much better results, and
> > > in fact these files already start with specially-formatted comments
> > > that instruct Emacs to process them as if they were Python files.
> > >
> > > This commit adds the equivalent special comments for vim.
> > >
> > > Signed-off-by: Andrea Bolognani <abologna@redhat.com>
>
> Given that we already have emacs mode-lines, I see no reason to
> not also have vim mode-lines, so regardless of the deeper discussion
> I think this is patch is fine to merge in the short term
>
>   Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>
>
> > Naming QAPI schema files .json even though their contents isn't was a
> > mistake.  Correcting it would be a pain.  If we correct it, then the
> > sooner the better.
> >
> > Renaming them to .py gives decent editor support out of the box.  Their
> > contents isn't quite Python, though: true vs. True, false vs. False.  Do
> > we care?  Only a few dozen occurences; they could be adjusted.
> >
> > Renaming them to .qapi would perhaps be less confusing, for the price of
> > "out of the box".
>
> IMHO, the critical rule is that if you a pick a particular file extension
> associated with an existing language, you absolutely MUST BE compliant
> with that language.
>
> We fail at compliance with both JSON and Python because we're actually
> using our own DSL (domain specific language).
>
> IOW if we're going to stick with our current file format, then we should
> be naming them .qapi. We can still use an editor mode line if we want to
> claim we're approximately equiv to another language, but we can't be
> surprised if editors get upset.
>
>
> The bigger question is whether having our own DSL is justified ?
>
> I'm *really* sceptical that it is.
>
>
> We can't use JSON because it lacks comments. So we invented our own
> psuedo-JSON parser that supported comments, and used ' instead of "
> for some reason. We also needed to be able to parse a sequence of
> multiple JSON documents in one file. We should have just picked a
> different language because JSON clearly didn't do what we eneeded.
>
> You suggest naming them .py. If we do that, we must declare that they
> are literally Python code and modify them so that we can load the
> files straight into the python intepretor as code, and not parse
> them as data. I feel unhappy about treating data as code though.
>
>
> While JSON doesn't do what we need, its second-cousin YAML is a more
> flexible format. Taking one example
>
> ---
> ##
> # @ImageInfoSpecificQCow2:
> #
> # @compat: compatibility level
> #
> # ...snip...
> #
> # Since: 1.7
> ##
> struct: ImageInfoSpecificQCow2
> data:
>   compat: str
>   "*data-file": str
>   "*data-file-raw": bool
>   "*lazy-refcounts": bool
>   "*corrupt": bool
>   refcount-bits: int
>   "*encrypt": ImageInfoSpecificQCow2Encryption
>   "*bitmaps":
>     - Qcow2BitmapInfo
>   compression-type: Qcow2CompressionType
>
>
> Then we could use a regular off the shelf YAML parser in python.
>
> The uglyiness with quotes is due to the use of "*". Slightly less ugly
> if we simply declare that quotes are always used, even where they're
> not strictly required.
>
> struct: ImageInfoSpecificQCow2
> data:
>   "compat": "str"
>   "*data-file": "str"
>   "*data-file-raw": "bool"
>   "*lazy-refcounts": "bool"
>   "*corrupt": "bool"
>   "refcount-bits": "int"
>   "*encrypt": "ImageInfoSpecificQCow2Encryption"
>   "*bitmaps":
>     - "Qcow2BitmapInfo"
>   "compression-type": "Qcow2CompressionType"
>
> With the use of "---" to denote the start of document, we have no trouble
> parsing our files which would actually be a concatenation of multiple
> documents. The python YAML library provides the easy yaml.load_all()
> method.

We had the same issue in vdsm. Someone ported qemu "json" schema to vdsm,
probbay when the plan was to add C API to vdsm, which never happened.

My first patch to vdsm was fixing the parser for this "json" format,
because it used to
get in an endless loop if an unknown token was found. We hated this
format, and finally
replaced it with yaml. But we did not keep the comments since they
duplicate data
which is already in the json part, and not portable to other formats.

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

We don't use comments, so the yaml is portable to json or regular
python dict. In fact,
we use the schama in as a pickle of the parsed schema for 5 times
faster loading, which
is important since we use the schema in the command line client.

Having the comments part of the schema allows nice things like
verifying requests and
generating help messages directly from the schema. This is not a good
example before
our implementation is poor, but:

$ vdsm-client Host getDeviceList -h
usage: vdsm-client Host getDeviceList [-h] [arg=value [arg=value ...]]

positional arguments:
  arg=value   storageType: Only return devices of this type
              guids: Only return info on specific list of block device GUIDs
              checkStatus: Indicates if device status should be checked


              JSON representation:
              {
                  "storageType": {
                      "BlockDeviceType": "enum ['FCP', 'MIXED', 'iSCSI']"
                  },
                  "guids": [
                      "string",
                      {}
                  ],
                  "checkStatus": "boolean"
              }

optional arguments:
  -h, --help  show this help message and exit

vdsm-client knows nothing about vdsm API and we never have to change
it, because it generates
the command line interface and the help messages from the schema on
the fly, and its input and
output is json.

vdsm/client.py is similar, providing vdsm API without knowing anything
about the API, or requiring
changes when APIs are added or modified, because everything is done by
inspecting the schema:

>>> from vdsm import client
>>> c = client.connect("localhost")
>>> c.Host.getDeviceList(storageType="FCP", checkStatus=False)
[]
>>> print(c.Host)
<vdsm.client.Namespace object at 0x7fcda017fa58>
>>> print(c.Host.getDeviceList)
functools.partial(<bound method _Client._call of <vdsm.client._Client
object at 0x7fcda757e0f0>>, 'Host', 'getDeviceList')

I think inventing DSLs and developing tools is wrong. Use standard
format and tools and spend
time on the core of the project.

Nir