Skip to content

[TOPIC: DTS] dts: bindings: Have 'required: true/false' instead of 'category: ...' (take two, edtlib version) #18723

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

Merged
merged 1 commit into from
Aug 28, 2019

Conversation

ulfalizer
Copy link
Collaborator

@ulfalizer ulfalizer commented Aug 28, 2019

The 'category: required/optional' setting for properties is just a
yes/no thing. Using a boolean makes it clearer, so have
'required: true/false' instead.

Print a clear error when 'category:' is used:

edtlib.EDTError: please put 'required: true' instead of 'category:
required' in 'properties: foo: ...' in
test-bindings/sub-node-parent.yaml - 'category' has been removed

The old scripts in scripts/dts/ ignore this setting, and only print a
warning if 'category: required' in an inherited binding is changed to
'category: optional'. Remove that code, since the new scripts already
have the same check.

The replacement was done with

git ls-files 'dts/bindings/*.yaml' | xargs sed -i \
    -e 's/category:\s*required/required: true/' \
    -e 's/category:\s*optional/required: false/'

dts/binding-template.yaml is updated as well.

@ulfalizer
Copy link
Collaborator Author

ulfalizer commented Aug 28, 2019

Another thing I've been thinking about:

required: true would probably be a better default than required: false, since there are a lot more required properties than optional ones. It would completely get rid of required: from many bindings.

The default when required: is missing doesn't seem to be documented now either, so there might be properties that ought to be required that aren't.

Any thoughts?

I also considered the json-schema version, where you list all the required properties separately at the top level:

required:
    - foo
    - bar
    - baz

I couldn't think of any technical advantages compared to what we have though, and there are a few disadvantages:

@ulfalizer ulfalizer changed the title dts: bindings: Have 'required: true/false' instead of 'category: ...' (take two, edtlib version) [TOPIC: DTS] dts: bindings: Have 'required: true/false' instead of 'category: ...' (take two, edtlib version) Aug 28, 2019
The 'category: required/optional' setting for properties is just a
yes/no thing. Using a boolean makes it clearer, so have
'required: true/false' instead.

Print a clear error when 'category:' is used:

    edtlib.EDTError: please put 'required: true' instead of 'category:
    required' in 'properties: foo: ...' in
    test-bindings/sub-node-parent.yaml - 'category' has been removed

The old scripts in scripts/dts/ ignore this setting, and only print a
warning if 'category: required' in an inherited binding is changed to
'category: optional'. Remove that code, since the new scripts already
have the same check.

The replacement was done with

    git ls-files 'dts/bindings/*.yaml' | xargs sed -i \
        -e 's/category:\s*required/required: true/' \
        -e 's/category:\s*optional/required: false/'

dts/binding-template.yaml is updated as well.

Signed-off-by: Ulf Magnusson <[email protected]>
@galak galak merged this pull request into zephyrproject-rtos:topic-dts Aug 28, 2019
@galak galak deleted the clearer-category branch August 28, 2019 03:38
@galak
Copy link
Collaborator

galak commented Aug 28, 2019

Another thing I've been thinking about:

required: true would probably be a better default than required: false, since there are a lot more required properties than optional ones. It would completely get rid of
required: from many bindings.

The default when required: is missing doesn't seem to be documented now either, so there might be properties that ought to be required that aren't.

Did we require category to be set before? Hmm, wonder if its better to keep it explicit and enforce that 'required' be provided.

Any thoughts?

Kinda related, we need to clean up the overriding of required. I think there's a binding that fails today, but it isn't used in any default code so we don't see the issue.

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