Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix broken YAML #74

Merged
merged 5 commits into from
Jul 7, 2023
Merged

Fix broken YAML #74

merged 5 commits into from
Jul 7, 2023

Conversation

otterdahl
Copy link
Contributor

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
Copy link
Contributor

emiltin commented Jul 7, 2023

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
Copy link
Contributor

emiltin commented Jul 7, 2023

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

@otterdahl
Copy link
Contributor Author

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

@emiltin
Copy link
Contributor

emiltin commented Jul 7, 2023

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unresolved merge conflict?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, fixed now

@emiltin
Copy link
Contributor

emiltin commented Jul 7, 2023

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
Copy link
Contributor

emiltin commented Jul 7, 2023

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

@emiltin
Copy link
Contributor

emiltin commented Jul 7, 2023

i think this is ready for merging

@emiltin emiltin merged commit 426dc18 into master Jul 7, 2023
@emiltin emiltin deleted the fix_on_off branch July 7, 2023 07:50
@otterdahl
Copy link
Contributor Author

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
Copy link
Contributor

emiltin commented Jul 7, 2023

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:

yaml/libyaml#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants