-
Notifications
You must be signed in to change notification settings - Fork 157
Add spec text for scoped contexts #445
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
Conversation
This ended up being a pretty small change to Expansion and Compaction algorithms. Solves three different open issues. Note that #426 would have addressed this too, but it seems to be lacking support. |
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 required changes are indeed surprisingly small.
What concerns me is that a conformant JSON-LD 1.0 processor would silently process a document with scoped contexts by simply ignoring them. This would yield to a situation in which two JSON-LD processors would process and thus interpret a document differently. This is particularly dangerous if a default vocab is set at the top level of a document as it is the case for almost every usage of schema.org. I think we should tweak the design so that 1.0 processors would reject such documents. What do you think?
@@ -876,7 +877,7 @@ | |||
<li>If <em>value</em> contains the key <code>@type</code>: | |||
<ol class="algorithm"> | |||
<li>Initialize <em>type</em> to the value associated with the | |||
<code>@type</code> key, which must be a <a>string</a>. Otherwise, an | |||
<code>@type</code> key, which MUST be a <a>string</a>. Otherwise, an |
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.
This is a property of a conformant document, not a requirement of the algorithm. So shouldn't this remain a lowercase must?
(same elsewhere)
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.
Debatable, but that seems to be were we came down in the 1.0 document, so we'll use "must".
<li>Invoke the <a href="#context-processing-algorithm">Context Processing algorithm</a> | ||
using the <a>active context</a> and <em>context</em> as <a>local context</a>. | ||
If any error is detected, an | ||
<a data-link-for="JsonLdErrorCode">invalid scoped context</a> errors |
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.
errors -> error
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 really want swallow the error though? Or should we start supporting nested errors?
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.
IMO, we go overboard on errors as it is. This tells the author that the problem is with a scoped context, rather than with the context in general, so it seems more useful this way to me.
I believe From 1.0:
|
Oops, never mind, wrong change; let me check more; we were pretty careful for looking at such things. |
Looks like you're right. Create Term Definition doesn't look for a constrained list of term definition properties, which it should have. Perhaps we could consider an errata for 1.0 to error on "invalid term definition" if it contains anything other than Do you have a suggestion for how to handle this? We need to be able to do new things for 1.1. |
@lanthaler thanks for your review! I addressed your specific issues, and added wording about giving an error on unexpected keys in a term definition. Something like that could be considered for a 1.0 errata, but would need to investigate how to make that happen. Probably best to wait until we're closer to finishing 1.1 in case any other things to be addressed in 1.0 come up. |
+1 to merging this. If we can leave an issue open to try and address the 1.0 vs. 1.1 issue in a better way that would be a good thing. |
Term mappings require the value to be null, a string or an object. So we could trigger an issue by a value that is a number (version), a boolean (flag), or an array (list of required features). Something like
or
|
Of there, the That said, we might want to have a specific version announcement feature in the context. Perhaps setting the processingMode directly, in which case: {
"@context": {
"@processingMode": "json-ld-1.1",
...
}
} would be a better way to do this, and be consistent with our description of The JSON-LD Framing 1.1 document specifies using "json-ld-framing-1.1-expand-frame" as the processing mode to be passed to the Expansion algorithm when processing the JSON-LD Frame (this should probably change to "json-ld-1.1-frame-expansion", or similar, once framing is rolled into the API document). We could consider requiring that frames include this |
@lanthaler If you think your review concerns are satisfied, can you complete the review? I'm going to add an issue on adding |
Presume your issues have been resolved, given the open issue #446 on version announcement.
Sounds good |
to syntax and API documents along with compaction and expansion tests.
Fixes #247. Fixes #262. Fixes #369.