-
-
Notifications
You must be signed in to change notification settings - Fork 172
Vocab data v2 #316
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
Vocab data v2 #316
Conversation
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.
Overall this is a great change. I have some comments/suggestions around the fine points of URI and media type usage, and on how certain error cases can or can't be detected.
With the latest edits, I have a few more changes to make to the implementation.
|
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 added a few more comments and nitpicks, but nothing that would be considered blocking even if accepted. I'm excited to see this!
- [Relative JSON Pointers](https://json-schema.org/draft/2019-09/relative-json-pointer.html) | ||
- URI references per [RFC 3986](https://datatracker.ietf.org/doc/html/rfc3986), potentially with JSON Pointer fragments | ||
- URI-encoded JSON Pointer identifier per [RFC 6901, §6](https://www.rfc-editor.org/rfc/rfc6901#section-6) | ||
- Absolute URIs per [RFC 3986](https://datatracker.ietf.org/doc/html/rfc3986), optionally with a JSON Pointer fragment identifier |
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.
nitpick: by definition, absolute URIs do not have fragments. I think the language we use in the JSON Schema spec is "full URI (with scheme)".
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.
"with" here means "additionally"
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.
reworded
- [Relative JSON Pointers](https://json-schema.org/draft/2019-09/relative-json-pointer.html) | ||
- URI references per [RFC 3986](https://datatracker.ietf.org/doc/html/rfc3986), potentially with JSON Pointer fragments | ||
- URI-encoded JSON Pointer identifier per [RFC 6901, §6](https://www.rfc-editor.org/rfc/rfc6901#section-6) |
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.
If instead you say "fragment-only URI using URI-encoded JSON Pointers per..." then you can get rid of the statement about parsing order further down, because fragment-only URIs, full URIs, JPs, and RJPs do not overlap at all in syntax. Unless you're trying to support some non-fragment-only references? But that seems confusing.
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.
Sure, I can make this update. The only catch is I am trying to support full URIs + JP fragments (https://example.com/data#/foo
).
{ "format": "json-pointer" }, | ||
{ "format": "relative-json-pointer" } | ||
{ "format": "relative-json-pointer" }, | ||
{ "format": "uri-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.
You can add "pattern": "^#"
to restrict this to fragment-only URIs if you go with that suggestion I made earlier.
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.
Do we have a format for full (absolute) URI that could also include a JP fragment? I'd have to split out uri-reference
to support "JP fragment URI" and "full URI with (+) optional JP fragment" as separate items (which is fine, but I'd have to do it).
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've split this to iri
and iri-reference
-with-pattern
.
(I've also updated to use IRI instead of URI.)
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.
Do we have a format for full (absolute) URI that could also include a JP fragment?
@gregsdennis one of my many half-designed format
-replacement vocabularies would be capable of this 🙃
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.
Latest changes look good!
Resolves some conversations with @handrews in the JSON Schema Slack workspace.
Change summary:
This brings
data
resolution in closer alignment with$ref
(and potentially solves some other issues.