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

Allow boolean subschemas everywhere. #128

Closed
wants to merge 1 commit into from

Conversation

handrews
Copy link
Contributor

@handrews handrews commented Nov 4, 2016

This implements issue #101

@handrews
Copy link
Contributor Author

Added a commit that puts JSON Reference in the meta-schema. This addition does not change any specified behavior at all, it just further brings the meta-schema in line with the latest draft. I added it here because it was easier than having to merge two different subschema fixes.

@handrews
Copy link
Contributor Author

@awwright @Relequestual any concerns holding this up?

@epoberezkin
Copy link
Member

lgtm

as a side comment - we are on the right path of differentiating schemas and subschemas :)

"anyOf": [
{"$ref": "#"},
{"type": "boolean"},
{"$ref": "http://json-schema.org/draft/hyper-schema#/definitions/jsonReference"}
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks! Fixed.

@@ -3,10 +3,24 @@
"$schema": "http://json-schema.org/draft/schema#",
"description": "Core schema meta-schema",
"definitions": {
"jsonReference": {
Copy link

Choose a reason for hiding this comment

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

Shouldn't the property "$ref" be required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'll fix that, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

This is one alternative for issue json-schema-org#101.

If accepted, the meta-schema change will come separately after
PR json-schema-org#168 is merged.
@handrews
Copy link
Contributor Author

@awwright @Relequestual @epoberezkin

I have taken the meta-schema changes out of this and will re-add once PR #168 is accepted, at which point the changes will be simpler.

I will do the same with PR #167 (the version of this that includes root schemas), which I definitely prefer to this version now. So if there's any consensus on that please comment so I can close this version.

@handrews
Copy link
Contributor Author

handrews commented Dec 1, 2016

@awwright if you're pretty sure that #167 is the better approach (and at this point I prefer it) I can close this one out.

@@ -189,7 +189,7 @@
<t>
A JSON Schema document, or simply a schema, is a JSON document used to describe an instance.
A schema is itself interpreted as an instance.
A JSON schema MUST be an object.
A JSON Schema document MUST consist of an object, known as the root schema.
</t>
Copy link
Member

Choose a reason for hiding this comment

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

This language is not correct. "Consists of" means "has parts". Previous "must be" is correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's fixed in #167, which currently has more support than this PR. If that changes I will backport @awwright's language fixes to this PR.

@@ -535,12 +532,11 @@
</t>
<t>
This keyword's value MUST be an object. Each property specifies a dependency.
Each dependency value MUST be an object or an array.
Each dependency value MUST be an array or a valid subschema.
</t>
Copy link
Member

Choose a reason for hiding this comment

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

I would add "array of strings"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's covered on line 542.

@awwright
Copy link
Member

awwright commented Dec 4, 2016

#167 was used instead.

@awwright awwright closed this Dec 4, 2016
@handrews handrews deleted the bool branch December 5, 2016 19:15
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.

None yet

4 participants