Skip to content

Fix recursion in hyper-schema.json #397

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 3 commits into from
Sep 8, 2017

Conversation

levbishop
Copy link
Contributor

@levbishop levbishop commented Sep 7, 2017

  • Fix recursion to cover contains (specifically mentioned in section 3.1 of the hyper-schema spec), and propertyNames (probably irrelevant, since none of the hyper-schema fields would apply, but feels like a better practise since the core spec refers to the hyper-schema definition for guidance on recursion when extending schemas (see also spec: remove/change extending meta-schemas recommendation #86))
  • Much more concise definition of recursion.

Fix recursion to include `contains` (specifically mentioned in section 3.1 of the hyper-schema spec), and `propertyNames` (probably irrelevant, since none of the hyper-schema fields would apply) 
Much more concise definition of recursion. Probably *too* concise.
@levbishop
Copy link
Contributor Author

Perhaps the recursion is a little too concise and the regexes should be expanded a bit from, eg, "^(any|all|one)Of$" to "^(anyOf|allOf|oneOf)$"?

Or perhaps the original verbose form is clearest and only needs contains and propertyNames added?

@levbishop levbishop changed the title Update hyper-schema.json Fix recursion in hyper-schema.json: c Sep 7, 2017
@handrews
Copy link
Contributor

handrews commented Sep 7, 2017

@levbishop thanks for submitting a PR! It's great to see a new name in the PR list.

Or perhaps the original verbose form is clearest and only needs contains and propertyNames added?

Yes, I think this is preferred. Many people use the meta-schemas as shorthand documentation. It's debatable as to how wise that is, but using patternProperties here is more than a little off-putting. I rarely see it used at all across several large projects, and not everyone is comfortable with regexes.

While this PR is fine (especially since you are clearly open to re-working it), most of the time it's better to file an issue first until you've been through a draft cycle and have a feel for how things land with various team members. But definitely no need for that here, if you change this to just at contains and propertyNames I'll be happy to merge it (I'm going to double check and see if there's a reason they were left out, but I'm pretty sure it was an accident)

Go back to the much less concise, but more readable, specification of the recursion.
@levbishop levbishop changed the title Fix recursion in hyper-schema.json: c [WIP] Fix recursion in hyper-schema.json: c Sep 8, 2017
fix whitespace issues
@levbishop
Copy link
Contributor Author

Reworked to verbose format for recursion (still slightly simplified relative to original).

@levbishop levbishop changed the title [WIP] Fix recursion in hyper-schema.json: c Fix recursion in hyper-schema.json: c Sep 8, 2017
@levbishop levbishop changed the title Fix recursion in hyper-schema.json: c Fix recursion in hyper-schema.json Sep 8, 2017
Copy link
Contributor

@handrews handrews left a comment

Choose a reason for hiding this comment

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

Thanks, this is great! Good catch removing the no-longer-needed anyOfs as well.

@handrews handrews merged commit 0f22800 into json-schema-org:master Sep 8, 2017
@handrews handrews added this to the draft-07 (wright-*-02) milestone Sep 8, 2017
@levbishop levbishop deleted the patch-1 branch September 9, 2017 15:58
@gregsdennis gregsdennis added clarification Items that need to be clarified in the specification and removed Type: Bug labels Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification Items that need to be clarified in the specification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants