-
-
Notifications
You must be signed in to change notification settings - Fork 348
Decouple Core from media types #1627
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
base: main
Are you sure you want to change the base?
Conversation
05becd0
to
54f0d18
Compare
Once I removed the language about media types (as well as some language about vocabularies that should have already been removed) there was little difference between these sections. I rewrote them to be more distinct and serve separate purposes. I'm pretty happy with the Abstract and Introduction, but wasn't quite sure what to put in the Overview. I chose to make it a general overview of how JSON Schema works. I think it works, but let me know if you're thoughts.
54f0d18
to
36390a7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good. I don't think the overview is redundant.
Just a couple comments.
describe compound values like arrays and objects. For example, to describe a | ||
JSON object, a schema can use the `type` keyword to declare that the value MUST | ||
be an object, and the `properties` keyword to apply separate schemas to each of | ||
the object's properties. This allows for the evaluation of a complex JSON | ||
document using a uniform recursive algorithm. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it would be good to add a note that the presence of properties
doesn't imply that the instance must be an object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that would be a reasonable thing to put in the definition of properties
, but I think it's getting too deep in the details for the overview section.
However, it could make sense to cover the general principle that some keywords only apply when the instance is of a particular type. But, don't think we want to get too into architectural principles in the overview. It could balloon as we decide that other architectural principles should be mentioned as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The definition of properties
already says "if the instance is an object...". I'm not sure we really need to expand on that.
The example in this paragraph, however, seems to make a small implication that properties
requires that the instance is an object. That's just how I read it.
- If the media type of the schema is known and the media type defines a | ||
default dialect or a way to declare a dialect, the dialect MUST be | ||
determined by the rules of that media types. For example, the | ||
[application/schema+json] media type includes a `schema` parameter that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this pre-emptive reference to a media type that doesn't yet exist okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My plan was to reference (and publish) the draft included in this PR. I don't see a problem with referencing a draft. Then at some point in the future when it gets officially registered, we update the link to point to the official RFC or the IANA registry entry (which would link to the official RFC).
If you're not comfortable with that, we have two options.
- Remove the example. It can be added back in the future once the media type is officially registered.
- Add a note clarifying that the media type is not yet officially registered, but the intention is that it will be as soon as we can.
Although it isn't completely necessary, I'd prefer to keep the example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That fine. I just wanted to raise it.
When a schema is resolved over HTTP or another protocol that declares the media | ||
type of the response, implementations SHOULD refuse to evaluate schemas whose | ||
declared media type is not a known and supported JSON Schema media type such as | ||
[application/schema+json].[^10] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pre-emptive reference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this is a pretty important place to reference a media type that they should be using and leaving it out for now would be problematic. However, including a note that it's not official yet would make sense.
Resolves #1185
This PR moves us away from talking about JSON Schema as a media type and toward talking about it as a language that can be implemented in a number of media types. For example, there could be an
application/schema+yaml
as well asapplication/schema+json
.The media type definitions have been moved to a separate document written as an I-D with the intention of it being adopted by the HTTP-APIs IETF working group when we're ready. HTTP-APIs already has a version of this here, but this has been completely rewritten and I thought it would be better for it to be maintained here until we're ready. Specifically, we need to publish the spec first before we can register media types that reference them.
In addition to removing media type language where necessary, I also rewrote the Abstract, Introduction, and Overview. Once I removed the language about media types (as well as some language about vocabularies that we missed previously) there was little difference between these sections. I rewrote them to be more distinct and serve separate purposes. I'm pretty happy with the Abstract and Introduction, but wasn't quite sure what to put in the Overview. I chose to make it a general overview of how JSON Schema works. I think it works, but let me know your thoughts.
NOTE: This depends on #1624 and includes those commits, but that PR should be merged first and then this PR will should only contain the spec updates. For now, you can just look at the(The infrastructure changes have been merged now).md
files and ignore the rest as noise.