rsmp-nordic / rsmp_schema

JSON Schema for automatically validating RSMP messages
MIT License
1 stars 0 forks source link

Fix broken YAML #74

Closed otterdahl closed 1 year ago

otterdahl commented 1 year ago

There is a slight error with A0301 where 'on' and 'off' is written as a key value pair, but only the first part exists. E.g.

'on':
'off':

But needs to be:

'on': ''
'off': ''

An alternative would be to write it as [ 'on', 'off' ], but that is treated as an array. Both variants exists in the YAML. Perhaps we should only use key/value pairs?

emiltin commented 1 year ago

Enums can be specified either as an array or as a hash. Hash values are currently ignored when converting to json schema, only the keys are used: https://github.com/rsmp-nordic/rsmp_schema/blob/master/lib/rsmp_schema/convert/export/json_schema.rb#L111

So the resulting json schema when using an array and a hash is the same. But the hash variant mean we have a description of each enum key, which can be useful for documentation.

A hash with empty values is allowed, it simply means there is no description of the enum keys. But in that case it's cleaner to use an array, as is used in e.g. A0303:

values: ['on','off']

So the current sxl.yaml is technically correct, but I think we should either keep using a hash but add an explanation for the on and off enums key, or switch to an array.

Since A0303 has the same errormode attribute with an [on,off] enum, it would be nice to be consistent.

emiltin commented 1 year ago

If we don't like hashes with empty values, we can add an error or warning when the converter is run.

otterdahl commented 1 year ago

Then I prefer values: ['on','off']. I've updated the PR

emiltin commented 1 year ago

ok. i've fixed it in the same way for SXL<1.1. I also added an error if enum hashes has empty values (when you convert to json schema).

emiltin commented 1 year ago

by the way, yaml allows strings without quotes in arrays:

[banana,guava]

But on and off are special, because they are parsed as true and false, so quotes are needed:

https://yaml-online-parser.appspot.com/?yaml=-+%5Bon%2C+off%5D%0A-+%5B%27on%27%2C%27off%27%5D%0A&type=json

emiltin commented 1 year ago

on a side note: our checks do not catch bugs in the yaml, because they don't regenerate the json schema.

emiltin commented 1 year ago

i think this is ready for merging

otterdahl commented 1 year ago

But on and off are special, because they are parsed as true and false, so quotes are needed:

A side note: There may a possibility to adjust the behavior of the YAML parser in this regard. In PyYAML (python) it is possible to modify the implicit resolver to not parse 'on'/'off' as booleans. sxl-tools (yaml2rst.py) does this.

emiltin commented 1 year ago

But on and off are special, because they are parsed as true and false, so quotes are needed:

A side note: There may a possibility to adjust the behavior of the YAML parser in this regard. In PyYAML (python) it is possible to modify the implicit resolver to not parse 'on'/'off' as booleans. sxl-tools (yaml2rst.py) does this.

It seems on and off are treated as normal strings from YAML 1.2, but the yamllib c lib that is used in both ruby and python does not yet fully support 1.2:

https://github.com/yaml/libyaml/issues/20 https://github.com/yaml/libyaml/wiki/YAML-1.2

I prefer to stick to the yaml parsing spec, rather than use flags or patching to modify the behaviour.