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

"$vocabulary" and basic vocabulary support. #671

Merged
merged 16 commits into from
Dec 18, 2018

Conversation

handrews
Copy link
Contributor

@handrews handrews commented Nov 11, 2018

It is recommended that the commits in this PR be reviewed one at a time, to avoid formatting changes making things more complicated than they are. See also their individual commit messages.

Addresses #561, #564, #567

I expect the wording here to have to go through some revision, but as is often the case I decided I just needed to post something to get the conversation started.

Key points:

  • When declaring the use of a vocabulary, you can indicate whether it is required or not, but that is all

  • For now, I'd rather not mandate any sort of vocabulary description file. As we get feedback, we will no doubt learn what is best put there. On the other hand, if implementation maintainers would like to hook specific keyword detection into their extension mechanism, I could reconsider and add such a thing as another PR. *[EDIT: For a variety of reasons, I will probably try to do this now after all]** [EDIT TO EDIT: actually I probably won't, see further comments in the issue]

  • This stuff gets a bit complicated. As with recursive references, most people will not need to write these things or understand their details, so in my view a relatively complex approach is acceptable. Plus, I've been talking this up for nearly a year and no one has proposed anything simpler yet :-)

core.json Show resolved Hide resolved
jsonschema-core.xml Outdated Show resolved Hide resolved
jsonschema-core.xml Outdated Show resolved Hide resolved
jsonschema-core.xml Outdated Show resolved Hide resolved
jsonschema-core.xml Outdated Show resolved Hide resolved
jsonschema-core.xml Outdated Show resolved Hide resolved
jsonschema-core.xml Outdated Show resolved Hide resolved
jsonschema-core.xml Outdated Show resolved Hide resolved
jsonschema-core.xml Outdated Show resolved Hide resolved
@handrews
Copy link
Contributor Author

handrews commented Nov 12, 2018

As I've thought about this more, I've decided to go ahead and try writing up a vocabulary definition file, for reasons. eh, never mind this.

@handrews
Copy link
Contributor Author

In the most recent commit, I have tried to make it clear at the start of the $vocabulary section that $vocabulary, like pretty much all keywords in meta-schemas, applies to the schemas that the meta-schema describes. It is actually quite normal in that respect.

This was referenced Nov 14, 2018
All of those other sections were not intended to be subsections
of the "$vocabulary" keyword section.
Clarify both the mandatory standard way, and that there is
a non-interoperable option for implementation-specific functionality
or optimizations.
Per review feedback, explicit is probably better than implicit
here.  At least until we get more real-world feedback.  Easier
to relax it later if we need to than add it.
Since we now define two in this document.
It relates to the schemas described by the meta-schema, not
to the meta-schema itself.
Both of these should be considered required, plus remove the
wording implying that only the core vocabulary would be in
the default "$vocabulary" value.
@handrews
Copy link
Contributor Author

OK, I think I've addressed (either with changes or explanations) all of the feedback. @gregsdennis and @jgonzalezdr does this seem good? Keep in mind that we can continue to consider wording improvements through the final review period.

@awwright since it's been more than two weeks since I replied to your comment with no further response from you I'm assuming you're OK with things. Please speak up in the next couple days if not (or if you don't see this until after it is merged, just file an issue and I'll be happy to revisit it).

schema.json Outdated Show resolved Hide resolved
Copy link
Member

@gregsdennis gregsdennis left a comment

Choose a reason for hiding this comment

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

I like it.

jsonschema-core.xml Outdated Show resolved Hide resolved
jsonschema-core.xml Outdated Show resolved Hide resolved
jsonschema-core.xml Outdated Show resolved Hide resolved
jsonschema-core.xml Outdated Show resolved Hide resolved
Co-Authored-By: handrews <andrews_henry@yahoo.com>
@handrews handrews changed the base branch from rec-ref-anchor to master December 18, 2018 02:54
@handrews handrews merged commit 7395658 into json-schema-org:master Dec 18, 2018
@philsturgeon
Copy link
Collaborator

Wahhoooo! :D

@Relequestual
Copy link
Member

If we had a budget or such, this would be huge cause for celebration! =D

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

Successfully merging this pull request may close these issues.

8 participants